feat(Alert, Icon, Modal, NotificationDrawer): use custom CSS for default #8924
Conversation
|
Preview: https://patternfly-react-pr-8924.surge.sh A11y report: https://patternfly-react-pr-8924-a11y.surge.sh |
| @@ -87,6 +87,14 @@ export interface AlertProps extends Omit<React.HTMLProps<HTMLDivElement>, 'actio | |||
| ouiaSafe?: boolean; | |||
| } | |||
|
|
|||
| const variantStyle = { | |||
| default: styles.modifiers.custom, | |||
There was a problem hiding this comment.
I believe we want to rename this variant to match it's style name. Previously there was no style being applied with the "default". Now there is.
There was a problem hiding this comment.
I'm a bit hesitant because "custom" doesn't really make sense to me as a user-facing modifier name. The user didn't customize the component nor is the component "custom" (in the sense of having large differences compared to the base component) compared to other enum values.
The styles are meant to represent the default styling of the component even if the name isn't "default" right? I thought that's what the blurb in the issue text about the modifier name not matching the css name indicated.
There was a problem hiding this comment.
@mceledonia @mcoker wasn't custom the agreed upon name for these variants?
There was a problem hiding this comment.
@kmcfaul you can see the discussion in this issue
patternfly/patternfly#5175
There was a problem hiding this comment.
It doesn't look like anything was decided on in the linked issue. In core "default" may conflict with keywords but a "default" string value for react shouldn't conflict I don't think, that's why I thought we were keeping "default" in react.
There was a problem hiding this comment.
It helps a bit. I'm mostly stuck on the thought that a "custom" status enum doesn't mean anything on its own (like "success" indicates "green" or "warning" indicates "yellow").
Continuing with Alert as an example, if a user adds variant="custom" to get the pf-m-custom modifier, the user hasn't done anything to customize the component - pf-m-custom is a predefined/cyan styling. If they were to define a custom color (via css, no react prop for this) and icon (via customIcon prop), they should likely leave out the variant enum altogether. Which makes the enum value seem a bit redundant or confusing to me?
There was a problem hiding this comment.
Updating to custom to keep the PR moving and match core. Opened patternfly/pf-codemods#400 for codemod.
There was a problem hiding this comment.
@kmcfaul agree, though to touch on what @mcarrano said, I think the custom variant is more of a wildcard/"other" variant (can be used for anything that isn't one of the existing success, error, etc statuses) with a pre-defined set of colors PF offers for whatever that's used for, that is unique to that variant. And anywhere we offer a status message/thing (alert, modal, popover, icon, icon, notification drawer), this other variant is available and there is consistency with the presentation. And if a user wants it to be purple because their brand is purple, they can update the --pf-global--custom-color--[whatever] variable(s) and it will update any component that offers the custom variant. I think that's probably the main value in offering an extra status variant.
From there, if they want a unique alert (or modal, popover, etc) variant specifically, like you said, they could just leave off the variant and use their own icon and CSS for the colors.
There was a problem hiding this comment.
Ah ok, having pf-m-custom tied to the --pf-global--custom-color--{x} makes it make a bit more sense to me now. I'm still worried there will be confusion though because the word doesn't indicate the user to anything.
There was a problem hiding this comment.
Had to update the docs a bit to reference the new enum value so made a note there
packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx
Outdated
Show resolved
Hide resolved
kmcfaul
force-pushed
the
no-default
branch
2 times, most recently
from
56f2ae6
to
c9284b6
Compare
|
|
||
| | Variant | Description | | ||
| |---|---| | ||
| | Default | Use for generic messages with no associated severity | | ||
| | Custom | Use for generic messages that should have a custom color set by the associated CSS variable. Should be used when the message has no associated severity. | |
There was a problem hiding this comment.
👍 The description update helps!
What: Closes #8775
Updates to use the new 'custom' CSS styling for our default enums.