feat(Card): Consume penta tokens by tlabaj · Pull Request #10056

feat(Card): Consume penta tokens by tlabaj · Pull Request #10056 · patternfly/patternfly-react

Conversation

@edonehoo I added a new example and a secondary styling switcher to some examples. Can you please take a look.

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!

tlabaj linked an issue

Feb 2, 2024

that may be closed by this pull request

Choose a reason for hiding this comment

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

LGTM thanks

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 <a> there or should this change to use the button?

Choose a reason for hiding this comment

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

^^ questions above

Comment on lines 106 to 108

		const registerTitleId = (id: string) => {
		setTitleId(id);
		containsCardTitleChildRef.current = !!id;
		};

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 pf-v5-c-card's overflow property that was added? Removing overflow: auto on the card prevents that bug. If we need to have that overflow auto then we could update this particular example to pass to the Dropdown, popperProps={{appendTo: () => document.body}}, but may be worth looking into further as a followup for another solution if we can keep the menu appended as close to the trigger as possible

Choose a reason for hiding this comment

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

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 <a> there or should this change to use the button?

@srambach The consumer is responsible for adding the button to the title. They may add one one that is an <a>. We have controll of that from the react side . Is there a reason to not support <a> in a card title?

@srambach for that bug where the menu isn't escaping the Card container, looks like it's due to the pf-v5-c-card's overflow property that was added? Removing overflow: auto on the card prevents that bug. If we need to have that overflow auto then we could update this particular example to pass to the Dropdown, popperProps={{appendTo: () => document.body}}, but may be worth looking into further as a followup for another solution if we can keep the menu appended as close to the trigger as possible

@srambach can we look into this on the core side. We try and append the menu as close to the trigger as possible.