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

Adds on-close to pf-notification-drawer component #1415

Merged

Conversation

AllenBW
Copy link
Member

@AllenBW AllenBW commented Apr 2, 2018

closes https://bugzilla.redhat.com/show_bug.cgi?id=1554809

does not modify default behavior of clicking the bell to open and close tray

So we have this real narrow edge case of 20-30 pixels where yah can end up with this view:

screen shot 2018-04-02 at 4 12 04 pm

In efforts to avoid this purgatory, unable to close notifications, proposing we do this, add a notification list close button (what this pr does):

screen shot 2018-04-02 at 4 09 34 pm

@AllenBW AllenBW added the bug label Apr 2, 2018
@AllenBW AllenBW requested review from chalettu and himdel April 2, 2018 20:16
@AllenBW AllenBW added this to the Sprint 83 Ending Apr 9, 2018 milestone Apr 2, 2018
@AllenBW
Copy link
Member Author

AllenBW commented Apr 2, 2018

cc @serenamarie125 yah ok with this change?

@miq-bot
Copy link
Member

miq-bot commented Apr 2, 2018

Checked commit AllenBW@3f820b8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍪

Copy link

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

hmmm @AllenBW there are responsive states for the notification drawer in PF ... http://www.patternfly.org/pattern-library/communication/notification-drawer/

  • the drawer should always have an "X" ( I think this was a recent addition )
  • but check out the responsive nature in the code example above

What's the change you made here, just adding the "X" ?

@AllenBW
Copy link
Member Author

AllenBW commented Apr 2, 2018

@serenamarie125 Yep ! just adding it, didn't know it was a recent change! why yah were tagged in, goooood to know 👍

Copy link

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

@AllenBW I guess great minds think alike. Thank you for the "X" 👍

@serenamarie125
Copy link

@miq-bot add_labels ux/approved

@serenamarie125
Copy link

@miq-bot remove_labels ux/review

Copy link
Contributor

@himdel himdel left a comment

Choose a reason for hiding this comment

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

Makes sense :)

@himdel himdel merged commit c6c277c into ManageIQ:master Apr 5, 2018
@AllenBW AllenBW deleted the BZ/#1554809-addsnotificationlistclose branch April 5, 2018 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants