Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resuming deposit modal #138
Resuming deposit modal #138
Changes from 6 commits
4b32328
cbdf3ee
f9c2ab0
12ab0a1
fcdf8d2
3738bfa
be492cd
0da8188
b5a02c4
d0d4805
0543d88
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can only name it as
Resume
. We are in themodals
directory so it seems redundant to me. We had the same approach withMissingAccount
or other modals declared here. The same goes for the file name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can keep the file name without the suffix
modal
because we are in themodals
directory, but I recommend staying with the current naming convention of components because such a name says more about the component type and will not be confused with the name of a function or other component if it appears and is not modal. I would even recommend such a change in other places as eg:MissingAccountModal
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion moved to discord
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suffix may be useful when we import this modal and use it elsewhere, it adds clarity.
On the other hand we've already got bunch of components that don't have the suffixes:
ActionForm
,Overview
, etc.Also let's take
Details
list, should it be renamed toDetailsList
?Should
Pause
icon become aPauseIcon
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like a deeper discussion to establish the common patterns, that we shouldn't block this PR with, unless we agree on the pattern and decide to refactor it across the board in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I left some much more detailed thoughts in the Discord thread, but quick answers on the above two examples:
Pause
should bePauseIcon
, andDetails
should have a more descriptive name thanDetails
orDetailsList
! Details of what?)