-
Notifications
You must be signed in to change notification settings - Fork 38
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
Extract and refactor clear queue dialog #1419
Conversation
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 kept this generic ConfirmActionDialog
component even though it's only used by the ClearQueueDialog
. I think it's a good abstraction so it seemed worth keeping.
@@ -54,6 +55,7 @@ function Main() { | |||
)} | |||
|
|||
<SelectProposalContainer /> | |||
<ClearQueueDialog /> |
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.
Sure, this dialog only makes sense in the context of the Samples page, but it doesn't really matter since its state is managed globally. I prefer to have all the dialogs in one place. It might uncover opportunities to simplify the global state and combine a few dialogs together into more general abstractions.
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.
Yes, it would be nice to only have, if possible, one main dialog component
9e6677d
to
aa95233
Compare
Thanks ! |
aa95233
to
a112275
Compare
Still continuing to convert class components. Here I'm mainly focusing on the dialog that appears when clicking on the "Clear sample list" button to confirm the action. I'm creating a dedicated component for this dialog so it is self-contained and can be extracted from the
SampleListViewContainer
component.I'm also tweaking the UX of the dialog slightly:
warning
theme variantand unmounts the current sample, if any.The last point makes me wonder whether the naming of the "Clear sample list" button is right... The main action is in fact the clearing of the queue... I'll open an issue to discuss this.