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(Title, Text): apply penta updates #9889

Merged
merged 4 commits into from
Dec 20, 2023

Conversation

kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented Dec 13, 2023

What: Closes #9681

  • Updates Title to include new header modifier classes
  • Updates ListItem to include missing list item class (though this doesn't exist on react-styles object so its hardcoded, and doesn't have styling associated with it. LMK if I should leave this out).

Title:
React translates heading level to size modifier classes (pf-m-2xl) instead of directly using header modifiers (pf-m-h1).

Text:
React is pulling in the text decoration token that specifies underline incorrectly (--pf-t--global--link--text-decoration is set to underline in react but was updated to none in core). This should be resolved automatically with a core update. The other minor difference is that the pf-m-visited class alone does not apply the color change in the react example, the link must be clicked for the :visited state before the purple color is applied. The react example should probably say "Link example" instead of "Visited link example".
Edit - updated core fixed the underline issue.

List:
Icon font size differs between core and react due to outdated tokens (react translates down to --pf-t--global--icon--size--lg: 22px while core translates to --pf-t--global--icon--size--lg: var(--pf-t--global--icon--size--250) which is 16px). This should be resolved automatically with a core update.
Edit - was out of date with core locally and icons look okay now beyond the slight difference that svgs cause.

@patternfly-build
Copy link
Contributor

patternfly-build commented Dec 13, 2023

@tlabaj tlabaj requested review from a team, nicolethoen and mfrances17 and removed request for a team December 13, 2023 20:07
@@ -16,7 +16,7 @@ export const ListItem: React.FunctionComponent<ListItemProps> = ({
}: ListItemProps) => (
<li className={css(icon && styles.listItem)} {...props}>
{icon && <span className={css(styles.listItemIcon)}>{icon}</span>}
{children}
<span className={css('pf-v5-c-list__item')}>{children}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we avoid hard coding the classname here? use the object from react-styles instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no object entry for this class is the issue, because it's not applying any styles currently. I can remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, can we do something like what Adam did on this line: https://github.com/patternfly/patternfly-react/blob/main/packages/react-core/src/components/DualListSelector/DualListSelectorPane.tsx#L155

it avoids us having hard coded versioned prefixes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha updated! Also caught upon double checking core that the class only gets applied if an icon is present so updated the logic/snapshots too.

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Dec 14, 2023

Rebased with core and looks like that fixed the weird token differences I was noticing, I was either slightly behind locally or had a cached file somewhere interfering.

Copy link
Member

@srambach srambach left a comment

Choose a reason for hiding this comment

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

Things seem to look good - my only comment would be that it would be nice to have the Title examples in the same order as core has them.
👍

Copy link
Collaborator

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

Happy days. Thanks

Copy link
Contributor

@mfrances17 mfrances17 left a comment

Choose a reason for hiding this comment

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

LGTM!

@nicolethoen nicolethoen merged commit edac507 into patternfly:v6 Dec 20, 2023
13 checks passed
@kmcfaul kmcfaul linked an issue Jan 2, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consume core updates: Title, Text and List
7 participants