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

[#56802] Document the new component and use it to replace streamable_flash_banner #16640

Conversation

mrmir
Copy link
Contributor

@mrmir mrmir commented Sep 5, 2024

Ticket

https://community.openproject.org/work_packages/56802

What are you trying to accomplish?

Screenshots

What approach did you choose and why?

Merge checklist

  • Added/updated tests
  • Added/updated documentation in Lookbook (patterns, previews, etc)
  • Tested major browsers (Chrome, Firefox, Edge, ...)

@oliverguenther oliverguenther force-pushed the implementation/56802-document-the-new-component-and-use-it-to-replace-streamable_flash_banner branch 3 times, most recently from 3a36ee7 to cde811c Compare September 19, 2024 18:06
@oliverguenther oliverguenther marked this pull request as ready for review September 19, 2024 18:07
@oliverguenther oliverguenther marked this pull request as draft September 19, 2024 18:08
@oliverguenther oliverguenther force-pushed the implementation/56802-document-the-new-component-and-use-it-to-replace-streamable_flash_banner branch from cde811c to aa30bfa Compare September 20, 2024 12:38
@oliverguenther oliverguenther marked this pull request as ready for review September 20, 2024 12:46
Copy link
Contributor Author

@mrmir mrmir left a comment

Choose a reason for hiding this comment

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

Hi @oliverguenther,

I pushed some small docs changes that I noticed were broken. Also noticed an extra div in the base layout, potentially.

Behaviour wise it looks okay to me, but there is this issue - When you edit a meeting in a new tab that is already closed, you get the 'This meeting is not editable anymore.' flash. But this then gets replaced by the 'This page has been updated...' flash. Should that be happening? I thought we were going to do stackable flashes or whatever to allow more than one at a time.

It looks jumpy and broken sometimes imo because of the timed poll for the update flash, where in some instances the first flash is only there for a fraction of a second before being replaced.

I'm not sure what the solution is here, but maybe we could remove that first flash altogether and replace it with an instance of the second? As they are kind of doing similar things now.

app/views/layouts/base.html.erb Outdated Show resolved Hide resolved
@oliverguenther oliverguenther force-pushed the implementation/56802-document-the-new-component-and-use-it-to-replace-streamable_flash_banner branch from 35332ee to fb741a6 Compare September 23, 2024 11:41
@mrmir
Copy link
Contributor Author

mrmir commented Sep 23, 2024

Looks good to me now :)

@oliverguenther oliverguenther merged commit b766075 into dev Sep 23, 2024
13 checks passed
@oliverguenther oliverguenther deleted the implementation/56802-document-the-new-component-and-use-it-to-replace-streamable_flash_banner branch September 23, 2024 17:56
akabiru added a commit that referenced this pull request Sep 27, 2024
https://community.openproject.org/work_packages/58142

#16640 unified the primerized flash message rendering and updated to
a default overlayed flash. Hence we should remove `full: true` that is used in full width contexts which is
no longer the case.
akabiru added a commit that referenced this pull request Sep 27, 2024
…option

https://community.openproject.org/work_packages/58142

#16640 unified the primerized flash message rendering and updated to
a default overlayed flash. Hence we should remove `full: true` that is used in full width contexts which is
no longer the case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants