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

chore(NotificationBadge): remove deprecated isread prop #8626

Conversation

gitdallas
Copy link
Contributor

What: Closes #8090

@gitdallas gitdallas linked an issue Jan 30, 2023 that may be closed by this pull request
@patternfly-build
Copy link
Contributor

patternfly-build commented Jan 30, 2023

children,
variant = isRead ? 'read' : 'unread',
variant,
Copy link
Contributor

@tlabaj tlabaj Feb 8, 2023

Choose a reason for hiding this comment

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

I believe this should default to unread.
@mcoker, what are the implications of not setting a variant. It looks like no variant is styled as read.

@mmenestr should the default of the notification badge be unread?

Copy link
Collaborator

@mmenestr mmenestr Feb 8, 2023

Choose a reason for hiding this comment

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

@tlabaj hmmm the default in what sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mmenestr In terms of the variant read, unread or attention
https://www.patternfly.org/v4/components/notification-badge#basic

Copy link
Contributor

@mcoker mcoker Feb 8, 2023

Choose a reason for hiding this comment

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

@tlabaj @mmenestr default and read do look the same. The core docs do say to specify a modifier for the state, though the requirement to add a state may have been a requirement from the previous designs, and looking at the code, it looks like the read modifier and default (no modifier) are the same more by chance, not because it's coded that way on purpose. A read notification badge is basically just an icon.

Unless we want to potentially introduce a "default" (no modifier) state for the notification badge that's different from "read", I think either updating the styling to be read by default (we would remove .pf-m-read) or assigning a default value for variant (variant={NotificationBadgeVariant.read}) (like the variant prop, since a button also requires a modifier) would be fine.

A default/no-modifier badge could be something like

  • neither read or unread, maybe you haven't clicked on it, or there is no state change regardless what you do to it
  • we want a default state to be the regular icon button color (color-200) and the "read" color to be more grey, and/or italic (if there is text) or something

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

lgtm other than the question about the default. Currently no variant (or an invalid one) renders the component without a modifier and the core docs mention the component needs a specified state, so if we keep that requirement, I think we would add "read" as the default for variant

@gitdallas gitdallas force-pushed the feat/8090-NotificationBadge-remove-deprecated-prop branch from 6ed1ca5 to d1fae98 Compare February 9, 2023 04:42
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

👍👍

@tlabaj tlabaj merged commit d4af3ed into patternfly:v5 Feb 9, 2023
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • @patternfly/react-charts@7.0.0-alpha.5
  • @patternfly/react-code-editor@5.0.0-alpha.9
  • @patternfly/react-core@5.0.0-alpha.9
  • @patternfly/react-docs@6.0.0-alpha.10
  • @patternfly/react-table@5.0.0-alpha.9

Thanks for your contribution! 🎉

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.

[Notification badge] - [remove deprecated prop]
8 participants