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

[OP#50985] use primer flash (banner) for file storage administration settings #14213

Conversation

akabiru
Copy link
Member

@akabiru akabiru commented Nov 22, 2023

https://community.openproject.org/wp/50985

To test, create a storage

  • Support dismiss action (Switches from Flash to Banner as flash does not yet have a working dismiss action)

See: primer/view_components#2394

Screenshot 2023-11-23 at 1 34 14 PM

@akabiru akabiru force-pushed the implementation/50985-use-primer-flash-for-administration-settings-with-primer branch from c878c0e to 0ce0aff Compare November 22, 2023 14:34
@akabiru akabiru changed the title [OP#50985] use primer flash for administration settings with primer [OP#50985] use primer flash for file storage administration settings Nov 22, 2023
@akabiru akabiru changed the title [OP#50985] use primer flash for file storage administration settings [OP#50985] use primer flash (banner) for file storage administration settings Nov 23, 2023
@akabiru akabiru marked this pull request as ready for review November 23, 2023 11:01
@akabiru akabiru force-pushed the implementation/50985-use-primer-flash-for-administration-settings-with-primer branch 3 times, most recently from a6195e6 to 9a3db92 Compare November 23, 2023 12:34
@akabiru akabiru requested review from HDinger and a team November 23, 2023 13:06
Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

Hi @akabiru

The banners look really nice 👍 I have some remarks:

  • When I delete a storage there is still the old success message shown
  • When I edit a storage (e.g change the name), I don't see any banner. However when I click afterwards on any link in the sidebar, I see on the new page the old flash message "Successfully updated".
  • On mobile, the banner looks off. I assume that this happens because the content-wrapper has additional padding on mobile and the banner is a child of that. So we might have to move the banner a bit up in the DOM
Bildschirmfoto 2023-11-24 um 08 43 31

Some additional notes, that do not necessariliy require changes on this PR:

  • When I create a OneDrive storage, and press cancel in the "OAuth applications" section, the section simply closes and I see no success banner. (Same when canceling in the nextcloud setup). I don't know though if the banner here is the right behaviour, or whether it would be better to redirect to the index page and show the banner there. What do you think?
  • I was wondering whether we want to show a banner in the error cases as well? However, I saw that there is nothing designed for that in Figma either, so I am fine with leaving it out for now.
  • Just a note: It feels weird, that the dismiss icon is focused and the tooltip shown initially. But since this is probably coming from Primer (and is accessibility conform) I am fine with keeping it like this.
  • Currently the tooltip of the closing icon "Dismiss" is not translated. I know that this is coming from Primer, but I would like to have a follow-up ticket to change that.

@akabiru akabiru self-assigned this Nov 24, 2023
@akabiru akabiru force-pushed the implementation/50985-use-primer-flash-for-administration-settings-with-primer branch from 9a3db92 to 88c23a9 Compare November 30, 2023 12:14
@akabiru
Copy link
Member Author

akabiru commented Nov 30, 2023

Thank you fro the review @HDinger , some responses

* [x]  When I delete a storage there is still the old success message shown

Thanks, Done ✅ initially the designs didn't cover this but it makes sense 👍🏾

* [ ]  When I edit a storage (e.g change the name), I don't see any banner. However when I click afterwards on any link in the sidebar, I see on the new page the old flash message "Successfully updated".

With the unified form, this behaviour is expected. As soon as a storage is created- the label changes from "incomplete" to "completed"- this is the indicator that the first step is complete- note that the completion flash is only rendered after all steps in the form have been completed.

* [ ]  On mobile, the banner looks off. I assume that this happens because the `content-wrapper` has additional padding on mobile and the banner is a child of that. So we might have to move the banner a bit up in the DOM
Bildschirmfoto 2023-11-24 um 08 43 31

Good catch, the full_when_narrow: false prop should cover this but it doesn't seem to work. I'm also not sure we can (should) move the flash any further as moving it out of the content-wrapper broke the page. Seems ok for now but I can track this in a WP?

Some additional notes, that do not necessariliy require changes on this PR:

* [ ]  When I create a OneDrive storage, and press cancel in the "OAuth applications" section, the section simply closes and I see no success banner. (Same when canceling in the nextcloud setup). I don't know though if the banner here is the right behaviour, or whether it would be better to redirect to the index page and show the banner there. What do you think?

Ditto: The success banner is only rendered after successful completion- note that it's not a "success created" message, rather a reminder to activate the storage in projects, which is not possible if the storage has not been fully configured.

* [ ]  I was wondering whether we want to show a banner in the error cases as well? However, I saw that there is nothing designed for that in Figma either, so I am fine with leaving it out for now.

Good eye, error cases are covered in form validation errors- but for cases like delete failure (edge-case), it's will render a danger banner.

* [ ]  Just a note: It feels weird, that the dismiss icon is focused and the tooltip shown initially. But since this is probably coming from Primer (and is accessibility conform) I am fine with keeping it like this.
* [ ]  Currently the tooltip of the closing icon "Dismiss" is not translated. I know that this is coming from Primer, but I would like to have a follow-up ticket to change that.

Good eye, this is hardcoded atm- we would have to fix it. https://community.openproject.org/wp/51360

@akabiru akabiru force-pushed the implementation/50985-use-primer-flash-for-administration-settings-with-primer branch from c245272 to 6ff1ae5 Compare November 30, 2023 13:19
@akabiru akabiru requested a review from HDinger November 30, 2023 13:35
Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

Hi @akabiru

Good catch, the full_when_narrow: false prop should cover this but it doesn't seem to work. I'm also not sure we can (should) move the flash any further as moving it out of the content-wrapper broke the page. Seems ok for now but I can track this in a WP?

It is okay for me to extract the mobile issue into a separate ticket 👍 Good to merge then from my side 🥳

@akabiru
Copy link
Member Author

akabiru commented Dec 1, 2023

Hi @akabiru

Good catch, the full_when_narrow: false prop should cover this but it doesn't seem to work. I'm also not sure we can (should) move the flash any further as moving it out of the content-wrapper broke the page. Seems ok for now but I can track this in a WP?

It is okay for me to extract the mobile issue into a separate ticket 👍 Good to merge then from my side 🥳

Thank you! Tracked in https://community.openproject.org/wp/51370

@akabiru akabiru merged commit 25b432c into dev Dec 1, 2023
9 checks passed
@akabiru akabiru deleted the implementation/50985-use-primer-flash-for-administration-settings-with-primer branch December 1, 2023 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants