Skip to content

feat(NotificationList) -Automates notification creation for list example - #329

Merged
jeff-phillips-18 merged 1 commit into
patternfly:masterfrom
AllenBW:enhancement/notification-story
May 8, 2018
Merged

jeff-phillips-18 merged 1 commit into
patternfly:masterfrom
AllenBW:enhancement/notification-story

Conversation

@AllenBW

@AllenBW AllenBW commented May 4, 2018 •

Copy link
Copy Markdown
Contributor

Improves notification list example

here's the storybook of fun!

@coveralls

coveralls commented May 4, 2018 •

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 1283

  • 0 of 0 (NaN%) changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 74.251%

Totals Coverage Status
Change from base Build 1266: 0.0%
Covered Lines: 1692
Relevant Lines: 2082

💛 - Coveralls

@michaelkro

Copy link
Copy Markdown
Contributor

Hey @AllenBW, I think the root cause of this behavior is our use of array indices as keys. In this branch, if you change this to

key={notification.key}

the bug goes away. I also doubled checked this in V2V by using notification.data.id as keys in <NotificationList>, and things are now working as expected.

Not sure if this will help with our quickly disappearing toasts, but I think this takes care of this issue :)

Here are a couple links

  1. Docs
  2. Medium Article

@AllenBW

AllenBW commented May 7, 2018

Copy link
Copy Markdown
Contributor Author

@michaelkro Updated this example to mix a lil bit of original implementation with a more real world situation, and the bugfix yah mentioned! (storybook also updated)

michaelkro
michaelkro previously approved these changes May 7, 2018

@michaelkro michaelkro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This improves the story to reflect a more likely use case, where notifications are dynamically added to the list. Looks good!

@priley86

priley86 commented May 7, 2018

Copy link
Copy Markdown
Member

this feels like more of a chore(NotificationList): Automates notification creation for list example since you are just touching .stories.js file? thanks for looking at this @AllenBW @michaelkro ! I'm good w/ the change otherwise...

@AllenBW
AllenBW force-pushed the enhancement/notification-story branch from 10468de to 8b51339 Compare May 7, 2018 15:19
@AllenBW

AllenBW commented May 7, 2018

Copy link
Copy Markdown
Contributor Author

@priley86 Reworded to chore! 😋

@michaelkro michaelkro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎉

@serenamarie125

Copy link
Copy Markdown
Member

LGTM!

@serenamarie125 serenamarie125 reopened this May 7, 2018

@serenamarie125 serenamarie125 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oops - meant to approve not close 😊

@jeff-phillips-18
jeff-phillips-18 merged commit 7398b5a into patternfly:master May 8, 2018
@AllenBW
AllenBW deleted the enhancement/notification-story branch May 8, 2018 14:37
karelhala pushed a commit to karelhala/patternfly-react that referenced this pull request May 9, 2018
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.

6 participants