-
Notifications
You must be signed in to change notification settings - Fork 358
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(Card): Consume penta tokens #10056
Conversation
Preview: https://patternfly-react-pr-10056.surge.sh A11y report: https://patternfly-react-pr-10056-a11y.surge.sh |
@edonehoo I added a new example and a secondary styling switcher to some examples. Can you please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
content looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
Only thing I noticed between core & react is the menu toggle is using the old dropdown toggle css on core. The border around the menu toggle appearing in this PR should be automatically resolved when the menu toggle penta updates are merged in.
It looks like the menu isn't escaping the confines of the expandable/expandable with icon cards - does the menu need to be attached differently? Second thing, the disabled card in https://patternfly-react-pr-10056.surge.sh/components/card#clickable-and-selectable has the wrong text color on the title. In core that's an inline link button, and the correct text color is coming from the styling on a disabled button. Do we want to make a change in core to accommodate using an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^^ questions above
const registerTitleId = (id: string) => { | ||
setTitleId(id); | ||
containsCardTitleChildRef.current = !!id; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should be able to remove this registerTitleId
here and from the context (and its use within CardTitle), as well as the containsCardTitleChildRef
ref. It was really only being used to get the card title ID from CardTitle to set the aria-labelledby attribute in Card.
In CardTitle we should be able to remove the useEffect on line ~25, too.
@srambach for that bug where the menu isn't escaping the Card container, looks like it's due to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than @thatblindgeye's comment above LGTM
@srambach The consumer is responsible for adding the button to the title. They may add one one that is an |
@srambach can we look into this on the core side. We try and append the menu as close to the trigger as possible. |
I created an issue to address the title link color on disabled cards ✅ CSS |
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #9991
Codemod issue opened: patternfly/pf-codemods#560