-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Update button should change status on modified white label #12577
Update button should change status on modified white label #12577
Conversation
- swap position between users & white label so that user's inner form - does not interfere with white_label own position in outer form - modified spec so that lowermost user is clickable
1a84e1b
to
8616847
Compare
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.
Interesting. This is a quick fix that doesn't address the underlying problem: a nested form. But this is valid to solve the S2 bug quickly.
Hello @mkllnk , Yes, as you pointed out, I should have mentioned it is a quick fix. |
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.
Nice quick fix, thanks @cyrillefr
I feel like we keep running in this nested form issue, I am wondering if we need to raise a tech deb ticket to properly fix this page ? @openfoodfoundation/core-devs ?
{ name: 'users', icon_class: "icon-user", show: true }, | ||
{ name: 'white_label', icon_class: "icon-leaf", show: true }, | ||
{ name: 'users', icon_class: "icon-user", show: true }, | ||
{ name: 'connected_apps', icon_class: "icon-puzzle-piece", show: show_connected_apps }, |
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 would have though it would break connected apps as well, but I can see it has it's own nested form so all good there.
Probably. But it's also a UX challenge. It's currently (almost) all one form with one save bar. So you can make several changes in several tabs and then save it all together. If we separate that out and do one form per tab then it changes the UX and you have to submit then form before leaving the tab, unless you write some complicated JS to track multiple changed forms with the save bar. I actually think that it would be better UX to change one page at a time. Having unsaved changes in an invisible tab is confusing. This task is also related to bye-bye-AngularJS and BUU. Maybe we should check in with Mario about any implications? |
Hi @cyrillefr, Before staging
After staging
ResultsIt's looking good. I couldn't spot any problems. Thank you very much!! |
What? Why?
I swapped
users
withwhite label
. Theusers
part contains a form, that is, an inner form.Everything after
users
(in the outer form, the one that contains side buttons ) is therefore not effective anymore.NB
This is a quick fix, as there is still a nested form.
What should we test?
Administration
admin/enterprises
white label
You have unsaved changes
.Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates