Skip to content

fix(layouts): applied tokens#6453

Merged
mcoker merged 4 commits into
patternfly:v6from
mcoker:issue-6316
Mar 26, 2024
Merged

fix(layouts): applied tokens#6453
mcoker merged 4 commits into
patternfly:v6from
mcoker:issue-6316

Conversation

@mcoker

@mcoker mcoker commented Mar 21, 2024

Copy link
Copy Markdown
Contributor

fixes #6316

@patternfly-build

patternfly-build commented Mar 21, 2024

Copy link
Copy Markdown
Collaborator

@mcoker mcoker requested a review from srambach March 21, 2024 23:41

@srambach srambach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see some mixture of v5 and v6 in the examples and docs of flex, grid, and gallery. Do you want to change that now or catch it in the upcoming PR for all the v5s?

.ws-core-l-bullseye .pf-v6-l-bullseye,
.ws-core-l-bullseye .pf-v6-l-bullseye__item {
padding: .5rem;
border: 2px dashed #393f44;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On the borders for the examples, do you want to use a token so that it works better in dark theme? Maybe also the border width just because?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep! Superb suggestion.

@@ -10,7 +10,7 @@
@each $property, $property-value in $pf-v5-global--spacer-properties-map {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you want to go ahead and change this to v6 here and on line 295 of scss-variables.scss?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Heck yes!

@mcoker

mcoker commented Mar 22, 2024

Copy link
Copy Markdown
Contributor Author

@srambach I'll make that update in this PR 👍👍

@srambach srambach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The examples look beautiful with the tokens applied!

@thatblindgeye thatblindgeye left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Couple comments below, nothing blocking here though

.#{$flex} {
// Emit spacer css variables that map to requested spacer values
@include pf-v5-emit-properties($pf-v5-l-flex--variable-map);
@include pf-v5-emit-properties($pf-v6-l-flex--variable-map);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a TODO regarding updating mixins used in these layouts to a v6 prefix?

$pf-v5-l-flex--spacer-map: build-spacer-map();
$pf-v5-l-flex--gap-map: build-spacer-map("base", "none", "xs", "sm", "md", "lg", "xl", "2xl", "3xl", "4xl");
$pf-v5-l-flex--variable-map: build-variable-map("#{$pf-prefix}l-flex--spacer", $pf-v5-l-flex--spacer-map);
$pf-v6-l-flex--breakpoint-map: build-breakpoint-map();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not a blocker for this, but would we be able to use the pf-prefix instead of hardcoding the version number in these files

@mcoker mcoker merged commit 6b0ea7d into patternfly:v6 Mar 26, 2024
@patternfly-build

Copy link
Copy Markdown
Collaborator

🎉 This PR is included in version 6.0.0-alpha.107 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kmcfaul kmcfaul linked an issue Mar 27, 2024 that may be closed by this pull request
7 tasks
@mcoker mcoker deleted the issue-6316 branch January 7, 2025 16:48
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.

Layouts - apply tokens

4 participants