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

Error messages don't block bottom of page clicks #12567

Merged
merged 1 commit into from
Jun 20, 2024
Merged

Error messages don't block bottom of page clicks #12567

merged 1 commit into from
Jun 20, 2024

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented Jun 13, 2024 •
edited
Loading

What? Why?

The flash container was set to 100% width to center the messages on the screen. The messages were covering only part of the screen though. So the container beyond the actual message box was covering part of the page, blocking clicks on elements.

A new way of centering the container with CSS translate means that the width of the container can be the same as the content, not covering anyting accidentally.

And moving the whole container up instead of only moving the contained message allows us to interact with elements below the flash message as well.

What should we test?

  • Activate products_v3 feature.
  • Delete your payments methods and shipping methods.
  • Go to the products edit screen.
  • Observe the warning message at the bottom.
  • Make sure that you can interact with all visible elements around the error message.

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

@mkllnk
Error messages don't block bottom of page clicks
81a8ee5 
The flash container was set to 100% width to center the messages on the
screen. The messages were covering only part of the screen though. So
the container beyond the actual message box was covering part of the
page, blocking clicks on elements.

A new way of centering the container with CSS translate means that the
width of the container can be the same as the content, not covering
anyting accidentally.

And moving the whole container up instead of only moving the contained
message allows us to interact with elements below the flash message as
well.
@mkllnk mkllnk self-assigned this Jun 13, 2024
@mkllnk mkllnk added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label Jun 13, 2024
@mkllnk mkllnk marked this pull request as ready for review June 13, 2024 01:38
@mkllnk mkllnk requested a review from dacook June 13, 2024 01:38
dacook
dacook approved these changes Jun 13, 2024
View reviewed changes
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Great, thanks for solving this.

I haven't used translateX much and wonder if we will lose the flexibility we had with flexbox (like trying to support stacking of multiple flash messages). But I don't think we support that yet anyways.

app/webpacker/css/admin_v3/components/messages.scss
width: 100%;
left: 50%;
transform: translateX(-50%);
bottom: 3.5rem;
z-index: $flash-message-z-index;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we could just move the z-index rule to the child .flash, so the container wouldn't cover anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe that could have been a solution, too.

By the way, I noticed that the flash content seems to be there twice. Once in the flash block and then in a notice block which is perfectly overlaying the first one. It doesn't really matter right now but I guess that we want to clean that up at some point.

Copy link
Member

Choose a reason for hiding this comment

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

Oh dear, does that mean you have to click "dismiss" twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! I didn't think of that but just tested and you are right. You have to click twice. 🤦

That's another issue, low priority though.

@RachL
Copy link
Contributor

RachL commented Jun 19, 2024

@filipefurtad0 I'm not successful in reproducing the error 🙈 so I'm not sure how to test this one. Can you try on your side?

@drummer83 drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Jun 20, 2024
@drummer83
Copy link
Contributor

Hi @mkllnk,

I managed to reproduce the issue and test your solution.

Positive

  • It is now possible to interact with all elements which are not covered by the content, e.g. the page buttons. ✔️
    image

Negative

  • As Maikel pointed out, there are two identical messages displayed on top of each other. This requires the user to click 'Dismiss' twice in order to make the message(s) disappear.
  • After saving changes, the confirmation dialog is showed additionally. However it shifts only one of the two aforementioned messages to the side, which makes the screen look very messed up.
    image

Both of the negative effects above are present in master already, so no regression here. I will open an issue for this.

Conclusion

It's a clear improvement enabling the interaction with elements on the page. No regressions introduced by this PR.
Merging! 🚀
Thanks again!

@drummer83 drummer83 merged commit f9e1ff9 into openfoodfoundation:master Jun 20, 2024
54 checks passed
@drummer83 drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Jun 20, 2024
@drummer83 drummer83 mentioned this pull request Jun 20, 2024
Closed
@mkllnk mkllnk deleted the flash-css-fix-v3 branch June 20, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dacook dacook dacook approved these changes

Assignees

@drummer83 drummer83

@mkllnk mkllnk

Labels
feature toggled These pull requests' changes are invisible by default and are grouped in release notes
Projects
OFN Delivery board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix style rule of error message when enterprise does not have shipping or payment method activated
5 participants
@mkllnk @RachL @drummer83 @dacook @filipefurtad0