-
Notifications
You must be signed in to change notification settings - Fork 20
Dataset Edit Terms Integration #874
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
base: develop
Are you sure you want to change the base?
Conversation
…for mobile viewports
ekraffmiller
left a comment
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.
Hi @ChengShi-1, it looks really good! See comments about suggested changes.
Sorry it took so long for this review, please don't worry about it till you are back from vacation! :)
src/sections/dataset/dataset-action-buttons/edit-dataset-menu/EditDatasetMenu.tsx
Outdated
Show resolved
Hide resolved
src/sections/edit-dataset-terms/dataset-terms-tab/DatasetTermsTab.tsx
Outdated
Show resolved
Hide resolved
src/sections/edit-dataset-terms/edit-license-and-terms/EditLicenseAndTerms.tsx
Show resolved
Hide resolved
…. remove duplicated searchParams 5. dynamic value: CUSTOM
ekraffmiller
left a comment
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.
Hi @ChengShi-1, all your changes look great, I just had another thought about the UI when testing this.
Currently, a user can make an edit on one tab, then click over to another tab without saving changes. I could see where a user would be confused, and think that 'Save Changes' means to save the changes made in all tabs within Edit Dataset Terms page.
If the user goes to a second tab without saving the first tab, then the changes will be lost.
I think the best solution for the user experience is to update "Save Changes" so it saves the data from all the tabs, but I realize that might be a big change. An alternate approach would be to warn the user if they click on another tab while unsaved changes exist on the current tab. We do something similar in the UploadFiles page - if the user attempts to navigate away from the page, we show a popup warning them, and giving an option to "stay on the page".
What do you think? Does one solution seem easier to implement than the other?
|
@ekraffmiller I agree the button can be confusing, and acts different from JSF. Since each tab submits separately in its own API and use case now, adding an unsaved-changes warning is simpler. I can add that warning now, and if we want a single save across tabs later, I can tackle that next. |
sounds good, thanks! |
|
Hi Ellen @ekraffmiller , I've implemented the "unsaved changes" pop-up modal. It's ready for review. I wanted to call out one behavior that might be confusing: The "Leave without saving" button prevents the changes from being submitted to the backend. However, the UI itself still temporarily saves the changes locally within the current tab. If the user navigates away and then back, the unsaved changes will still appear in the UI. I am not sure if we want this local persistence. |
It looks good! I see what you mean, the changes are still there until you hit Save. I think that's ok, we can change the warning to "If you leave now, your changes on this tab won't be saved". The main thing is to make sure they save everything that they intend to save. |
ekraffmiller
left a comment
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.
looks good!
What this PR does / why we need it:
This feature will mainly focus on “Dataset Terms”, “Restricted Files + Terms of Access” UI implementation.(Guestbook is not implemented for now)
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
yes, should be added later
Is there a release notes or changelog update needed for this change?:
yes, should be added later