Skip to content
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

Add a warning dialog before merging #3818

Merged
merged 6 commits into from
Jul 27, 2023
Merged

Add a warning dialog before merging #3818

merged 6 commits into from
Jul 27, 2023

Conversation

CarolineDenis
Copy link
Contributor

@CarolineDenis CarolineDenis commented Jul 20, 2023

Fixes #3816

@CarolineDenis CarolineDenis linked an issue Jul 20, 2023 that may be closed by this pull request
@CarolineDenis CarolineDenis requested review from melton-jason and a team July 20, 2023 21:19
@CarolineDenis CarolineDenis marked this pull request as ready for review July 20, 2023 21:19
Copy link
Contributor

@carlosmbe carlosmbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works fine. I'd approve it but just want to ask something.

Is it possible for us to make the warning multiple lines. Have it stretch across the entire screen feels a bit more eye straining compared to have 3 lines in the middle of the screen. If not eye straining, then a slight inconvenience.

Although, I suppose that extra effort does make sure we're communicating the gravity of situation.

Screenshot 2023-07-20 at 4 26 31 PM

VS

Just a rough mock up. Once more this is just my personal opinion.

Screenshot 2023-07-20 at 4 37 32 PM

@maxpatiiuk
Copy link
Member

maxpatiiuk commented Jul 21, 2023

Is it possible for us ...

Yes. @CarolineDenis you can use the className={} prop on dialog for that. See dialogClassNames - one of them will work for you

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking this PR until #3816 (comment) is answered as that's quite important

Copy link

@bronwyncombs bronwyncombs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should change the way the warning dialog is remembered, or possibly just how it is related to the merge records dialog.

Currently, after the warning dialog is moved, the merge record dialog will be that size the next open. Further, the warning dialog will then conform to the size of the agent merge dialog despite changing the agent merge dialog before prompting the warning.

We could also opt to disable the ability to change the warning dialog size altogether if that's a more timely option.
(As per @carlosmbe comment above, we'd have to discuss what that size would be.)

Screen.Recording.2023-07-21.at.7.40.50.AM.mov

Copy link
Member

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a checkbox as @maxpatiiuk mentioned if it is not too difficult that says
"Don't remind me again" to the left of the Proceed button. Will this add too much complexity?

Copy link
Member

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we replace the Proceed with Merge again?

image

Second, could we put the checkbox to the left of the "Accept" button?
Third, could we rename the "Accept" button to "Proceed"?

image

Fourth and finally, could we have the "Merge Records" warning dialog be smaller and centered instead of very wide? Those are all my comments!

Looking good!

Copy link
Contributor

@carlosmbe carlosmbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Can confirm the new layout and Do Not Show Again Button work.

@CarolineDenis
Copy link
Contributor Author

NOTE: to have the check box 'Don't show this again' back => go to user-tools and clear cache

@maxpatiiuk
Copy link
Member

@CarolineDenis if you create a new dataset without uploading (using the New button in the data sets window) you will see a dialog, that includes such checkbox (with local storage memory)

Copy link
Member

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Looks good! One weird thing before we merge– you can't uncheck this box once it is checked?

Firefox, macOS Sonoma

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only minor things, besides the bug Grant mentioned

Copy link

@bronwyncombs bronwyncombs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Checkbox working and cache behaves as expected.

Screen.Recording.2023-07-27.at.8.51.12.AM.mov

The dialog is constrained horizontally but the size can still be adjusted vertically. Re-sizing in this way is remembered, but not by the merge records dialog.

Screen.Recording.2023-07-27.at.8.53.13.AM.mov

@CarolineDenis CarolineDenis merged commit 7a2ad33 into v7.9-dev Jul 27, 2023
8 of 9 checks passed
@CarolineDenis CarolineDenis deleted the issue-3816 branch July 27, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Warning dialog before merging
5 participants