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(deps): Bump v6 branch to latest core alpha #9774

Merged
merged 12 commits into from Nov 15, 2023

Conversation

tlabaj
Copy link
Contributor

@tlabaj tlabaj commented Oct 26, 2023

What: Closes #9411

Additional issues: towards #9811, #9808, and #9809

  • Bumped core alphas
  • Masthead: Removed backgroundColor prop since light background is now the default with no option to change it.
  • NotificationBadge: .pf-v5-c-notification-badge__count no longer has a style associated with it so I updated the code with hardcoded placeholder string.
  • PageSidebar: Removed Page sidebar theme prop since it only supports the default theme.
  • PageSection: Removed light, dark and `darker variant since the are no longer supported.
  • PageHeader (deprecated): removed isSelected prop since it is no longer supported in Page
  • Fixed generate tokens to work with new naming

Screenshots of tokens BEFORE:
Screenshot 2023-11-02 at 4 42 44 PM

image

Screenshots of tokens AFTER:
image

image

@tlabaj tlabaj requested review from srambach, a team, nicolethoen and mfrances17 and removed request for a team October 26, 2023 20:34
@patternfly-build
Copy link
Contributor

patternfly-build commented Oct 26, 2023

@tlabaj tlabaj marked this pull request as draft October 26, 2023 21:08
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.

just some nits, copy/paste error in multiple places :)

packages/react-core/package.json Outdated Show resolved Hide resolved
packages/react-docs/package.json Outdated Show resolved Hide resolved
packages/react-icons/package.json Outdated Show resolved Hide resolved
packages/react-styles/package.json Outdated Show resolved Hide resolved
packages/react-tokens/package.json Outdated Show resolved Hide resolved
@nicolethoen
Copy link
Contributor

it'd be good to double check that any global tokens in the core repo are getting translated appropriately into react-tokens, just like the other css variables.

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.

Notification badges don't look quite right - there's a weird outline and the hover is wrong. But these are also slightly still in flight along with some button work from @mcoker
Page is getting a scrollbar that isn't there in core. That might take a little more investigation.

@tlabaj tlabaj marked this pull request as ready for review November 2, 2023 20:06
@tlabaj tlabaj requested a review from kmcfaul November 2, 2023 20:22
Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

This is really cool!

couple quick questions:

I see that we are defining the token for blue_100 for example as:

export const _pf_t_color_blue_100 = {
  "name": "--pf-t--color--blue--100",
  "value": "#d9e9ff",
  "var": "var(--pf-t--color--blue--100)"
};
export default _pf_t_color_blue_100;

Will that eventually be renamed and somehow replace the coexisting global variable?:

export const global_palette_blue_100 = {
  "name": "--pf-v5-global--palette--blue-100",
  "value": "#bee1f4",
  "var": "var(--pf-v5-global--palette--blue-100)"
};
export default global_palette_blue_100;

EDIT: this might be exactly what your TODO in the PR description is referring to - so never mind :)

Also, the individual token files look good, but in patternfly_variables.js I see quite a few tokens that have value: undefined such as:

"_pf_t_global_color_nonstatus_orangered_100": {
      "name": "--pf-t--global--color--nonstatus--orangered--100",
      "value": "undefined",
      "values": [
        "--pf-t--color--orangered--200",
        "undefined"
      ]
    },

Is that expected?

@tlabaj tlabaj requested a review from srambach November 13, 2023 15:47
@srambach
Copy link
Member

I'm still seeing a difference in notification badge. I know there's an upcoming sprint to clean up notification badge but I don't know if there's a problem with what's in this PR or if it will be addressed. The workspace styles to make the dark background can be removed for sure.
image

Otherwise, it looks ok to me and I'm fine moving forward and circling back.

@tlabaj
Copy link
Contributor Author

tlabaj commented Nov 15, 2023

I'm still seeing a difference in notification badge. I know there's an upcoming sprint to clean up notification badge but I don't know if there's a problem with what's in this PR or if it will be addressed. The workspace styles to make the dark background can be removed for sure. image

Otherwise, it looks ok to me and I'm fine moving forward and circling back.

@srambach This can be addressed by the issue for notification badge #9811
The goal of the PR was to pull in alphas, make sure it builds and make sure Tokens are generated correctly.

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

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.

React tokens - consume core v6 alphas to enable penta development
6 participants