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

Fix close button of the notification does not work and modernize notification pop up #314

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

thessakranendonk
Copy link

@thessakranendonk thessakranendonk commented Nov 13, 2022

PR for issue #284 : The close button of the notification does not work

  • Fixed not closing of alert notification button.
  • Added a new Popup.jsx component that looks more modern and is more prominent on the page. This popup looks more like a modal and makes the background of the page slightly darker when the notification pops up. Please let me know if this component would be a good replacement for the current notification or if there are any UI improvement suggestions..
  • In Issue 284 there was some discussion of removing the closeable prop but I think this prop is useful here. If the prop is passed to the component it will automatically close a notification after a certain timeframe without the user having to click the close button. Personally I don't like having to close popups by mouse click only and therefore I think this prop should stay.

Before:
Screen Shot 2022-11-13 at 6 18 48 PM

After:
Screen Shot 2022-11-13 at 6 20 30 PM
Screen Shot 2022-11-13 at 6 20 46 PM
Screen Shot 2022-11-13 at 6 21 27 PM

@jennydaman jennydaman linked an issue Nov 14, 2022 that may be closed by this pull request
@jennydaman
Copy link
Collaborator

Hey @thessakranendonk this is impressive. I will review the design first and then peek at your code in a later comment.

Before implementing changes to design or other major features, I recommend doing mock-ups first so that we can discuss before investing effort.

The most striking design element to discuss here is color choice, but also we can talk a bit about border radius and iconography. The ChRIS project uses the Patternfly v4 design language and you should refer to its specification here:

https://www.patternfly.org/v4/guidelines/colors

  • The colors are attention seeking, but I don't think they fit Patternfly's color palette.
  • I don't think these notifications are important enough to warrant dimming the screen plus a popup.

You've done a lot to make the popups as noticeable as can be, but it comes at the tradeoff of a cohesive design language.

IMO header bars should not be used on pop-ups. Header bars are implemented for window managers of desktop operating systems for reasons such as providing a handle for window placement, displaying windows buttons such as close, and the window title. Here, there is only one functionality (providing space for the close button) so the header bar is wastefully taking up a lot of space (with a striking color).

For more information about header bars, my favorite design spec for them is the GNOME HIG. https://developer.gnome.org/hig/patterns/containers/header-bars.html

@jennydaman
Copy link
Collaborator

Tagging @mairin, you are always welcome to chat with us about design in #chris-frontend:fedora.im

@thessakranendonk
Copy link
Author

@jennydaman thank you for your feedback!! I will have another look at this and make some changes. Very helpful! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The close button of the notification does not work
2 participants