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

feat(Notification Badge): Adds the notification badge component #2342

Merged
merged 3 commits into from Jun 27, 2019

Conversation

@dlabaj
Copy link
Contributor

dlabaj commented Jun 24, 2019

What:
Adds the notification badge component to react core.

Additional issues:
#2021

dlabaj added 2 commits Jun 24, 2019
Added the notificatoin badge component.  Currently adding the typescript integreation tests.  Will
update the PR when they are done.

"fix #2021"
@dlabaj dlabaj requested review from jschuler, jessiehuff, tlabaj and redallen Jun 24, 2019
@dlabaj dlabaj self-assigned this Jun 24, 2019
@dlabaj dlabaj changed the title Fixes #2021: Notification badge component feat(notificationbadge): Notification badge component Jun 24, 2019
@dlabaj dlabaj changed the title feat(notificationbadge): Notification badge component feat(Notification Badge): Adds the notification badge component Jun 24, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jun 24, 2019

PatternFly-React preview: https://patternfly-react-pr-2342.surge.sh

}
onClick = () => {
this.setState({
isRead: true

This comment has been minimized.

Copy link
@redallen

redallen Jun 25, 2019

Contributor

Could go with isRead: !this.state.isRead to allow toggling.

This comment has been minimized.

Copy link
@dlabaj

dlabaj Jun 25, 2019

Author Contributor

Thanks @redallen I figured once you set it you wouldn't want to show unread again, but if we want I can update the example to have it toggle. Just let me know I'm fine either way.

};
this.onClick = () => {
this.setState({
isRead: true

This comment has been minimized.

Copy link
@redallen

redallen Jun 25, 2019

Contributor

Same with the example.

@tlabaj tlabaj added the css review label Jun 25, 2019
@tlabaj tlabaj requested review from christiemolloy and mcarrano Jun 25, 2019
@tlabaj tlabaj added the ux review label Jun 25, 2019
@tlabaj tlabaj assigned jschuler and unassigned dlabaj Jun 25, 2019
Copy link
Member

mcarrano left a comment

@dlabaj Visually this looks good, but I do have a question about behavior. This is intended to work as a toogle for opening and closing the notification drawer. So when the drawer is open it would display a selected state until the user clicks it again to close the drawer. It's hard to tell from the preview of the icon only, but it looks like this is coded to behave link a standard action button.

Here is the latest visual spec: https://marvelapp.com/project/4090736/

@dlabaj

This comment has been minimized.

Copy link
Contributor Author

dlabaj commented Jun 27, 2019

@mcarrano I don't have access to that visual spec above

@dlabaj

This comment has been minimized.

Copy link
Contributor Author

dlabaj commented Jun 27, 2019

@mcarrano Right now this is an external component that can also be used outside of the notification drawer. Is the circle around it when it's toggled on suppose to be shown when it's not used with the notification drawer?

@christiemolloy @mcoker I don't see the toggled on circle in the core example. https://pf4.patternfly.org/components/NotificationBadge/examples/ Is this being added in a modifier that we will get later?

@dlabaj

This comment has been minimized.

Copy link
Contributor Author

dlabaj commented Jun 27, 2019

@mcarrano Spoke with Mary and she's going to add that selected state when the drawer is open to this issue patternfly/patternfly-next#1832. When we get this in core and it is time to add it to react we will add that to the notification badge in the masthead at that time. Does that sound good?

@christiemolloy

This comment has been minimized.

Copy link
Member

christiemolloy commented Jun 27, 2019

This looks great @dlabaj and we hadn't addressed the circle background behind the icon yet because we still need to discuss how to best implement it in Core. I'm going to create a separate issue for it because patternfly/patternfly-next#1832 is large and addresses different concepts. Issue here: patternfly/patternfly-next#1998 ... @rachael-phillips making you aware.

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Jun 27, 2019

@dlabaj @christiemolloy this all makes sense and thanks for identifying the approach here. I that case I will go ahead and approve of this PR.

@mcarrano mcarrano self-requested a review Jun 27, 2019
Copy link
Member

mcarrano left a comment

I'm good to merge this when ready.

@mcarrano mcarrano added ux approved and removed ux review labels Jun 27, 2019
@tlabaj
tlabaj approved these changes Jun 27, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit a0e7965 into patternfly:master Jun 27, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.