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.

Sign up for GitHub

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

Jump to bottom

Update button should change status on modified white label #12577

Merged
merged 1 commit into from
Jun 20, 2024
Merged

Update button should change status on modified white label #12577

merged 1 commit into from
Jun 20, 2024

Conversation

cyrillefr
Copy link
Contributor

@cyrillefr cyrillefr commented Jun 17, 2024 •
edited
Loading

What? Why?

I swapped users with white label. The users 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?

  • Log in as an enterprise owner /admin
  • Administration
  • go to admin/enterprises
  • choose a company
  • click on the white label
  • one should be able to check/uncheck and see the You have unsaved changes.
  • Entering text in the Trix editor should not raise a JS exception anymore (open the web dev tools).

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@cyrillefr
Update button should change status on modified white label
8616847 
- 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
@cyrillefr cyrillefr force-pushed the WhiteLabelButtonAlwaysInactive branch from 1a84e1b to 8616847 Compare June 17, 2024 15:11
mkllnk
mkllnk approved these changes Jun 18, 2024
View reviewed changes
Copy link
Member

@mkllnk mkllnk left a 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.

@cyrillefr
Copy link
Contributor Author

Hello @mkllnk ,

Yes, as you pointed out, I should have mentioned it is a quick fix.
If another menu item is added, this issue will come again.
Is there some kind of to-do list or the best way not to forget it is to make a new issue ?

rioug
rioug approved these changes Jun 18, 2024
View reviewed changes
Copy link
Collaborator

@rioug rioug left a 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 ?

app/helpers/admin/enterprises_helper.rb
Comment on lines -69 to 71
{ 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 },
Copy link
Collaborator

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.

@mkllnk
Copy link
Member

mkllnk commented Jun 19, 2024

I am wondering if we need to raise a tech deb ticket to properly fix this page ?

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?

@drummer83 drummer83 self-assigned this Jun 20, 2024
@drummer83 drummer83 added no-staging-AU A tag which does not trigger deployments, indicating a server is being used pr-staged-au staging.openfoodnetwork.org.au and removed no-staging-AU A tag which does not trigger deployments, indicating a server is being used labels Jun 20, 2024
@drummer83
Copy link
Contributor

Hi @cyrillefr,
Thank you for this quick fix. I have tested your solution.

Before staging

  • The Update button remains disabled.
  • There is a js error in the browser console.

image

After staging

  • The Update button becomes active upon changes.
  • There is no js error.
  • Tested with admin v3 and with current design.
  • Tested for super admin and enterprise users.
  • Verified that the neighboring tabs are still working.

image

Results

It's looking good. I couldn't spot any problems.

Thank you very much!!
Merging! 🚀

@drummer83 drummer83 merged commit 25d375b into openfoodfoundation:master Jun 20, 2024
54 checks passed
@drummer83 drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Jun 20, 2024
@filipefurtad0 filipefurtad0 added the user facing changes Thes pull requests affect the user experience label Jun 20, 2024
@cyrillefr cyrillefr deleted the WhiteLabelButtonAlwaysInactive branch June 22, 2024 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkllnk mkllnk mkllnk approved these changes

@rioug rioug rioug approved these changes

Assignees

@drummer83 drummer83

@cyrillefr cyrillefr

Labels
user facing changes Thes pull requests affect the user experience
Projects
OFN Delivery board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[White label] Update button always inactive (can't make changes)
5 participants
@cyrillefr @mkllnk @drummer83 @rioug @filipefurtad0