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

Toolbar component review #6701

Closed
mcoker opened this issue May 28, 2024 · 3 comments · Fixed by #6838
Closed

Toolbar component review #6701

mcoker opened this issue May 28, 2024 · 3 comments · Fixed by #6838

Comments

@mcoker
Copy link
Contributor

mcoker commented May 28, 2024

Looking at some changes in #6057 and issues that have resulted, the toolbar styles should be reviewed as a whole for feature parity with react and cleaned up wherever possible. For example, it was determined in patternfly/patternfly-react#10418 (comment) that we didn't need the .pf-m-margin-inline-[start/end]-[none/xs/sm/md/lg/xl/2xl/3xl/4xl]-on-[sm/md/lg/xl/2xl] modifiers that were added. A quick test of pulling that out slims the toolbar stylesheet down from 2921 lines and 128k in size to 2273 lines and 96k. For comparison, the v5 toolbar stylesheet is around 50k, and the v6 stylesheet around 130k.

--

Starting with the modifiers that generate a lot of CSS, review each of them for the following

  • Is the feature required? For example, it's required structurally, it's an approved user request or enhancement for v6, it's a pre-existing feature (that was in core and react in v5) that doesn't need to change.
    • Remove what is clearly not needed, make a note of anything else if there are questions.
  • Is it in react? Whether it's new for v6 or was in v5, if there is no react support for a feature, make a note of it and let's review and see if it's something we need to keep or can be removed.
  • Make sure the feature is documented (there are appropriate examples and classes/what-not are in the docs table), and that the documentation is accurate.

Here is a rough list of the modifiers in question, but verify this in case there are any I missed:

Hidden/visible - supported on content, content-section, group, item, group.pf-m-label-group-container, group.pf-m-label-group

Why is it on .#{$toolbar}__group 3 times?

.#{$toolbar}__content-section,
.#{$toolbar}__group,
.#{$toolbar}__item,
.#{$toolbar}__group.pf-m-label-group-container,
.#{$toolbar}__group.pf-m-label-group {
@include pf-v6-hidden-visible(flex);

.#{$toolbar}__content {
@include pf-v6-hidden-visible(grid);
}

Width/min-width vars

.#{$toolbar}__item {
@include pf-v6-build-css-variable-stack("--#{$toolbar}__item--Width--base", "--#{$toolbar}__item--Width", $pf-v6-c-toolbar--breakpoint-map);
@include pf-v6-build-css-variable-stack("--#{$toolbar}__item--MinWidth--base", "--#{$toolbar}__item--MinWidth", $pf-v6-c-toolbar--breakpoint-map);

Alignment on item and group

&.pf-m-align-start#{$breakpoint-name} {
margin-inline-start: 0;
}

&.pf-m-align-center#{$breakpoint-name} {
margin-inline-start: auto;
margin-inline-end: auto;
}

&.pf-m-align-end#{$breakpoint-name} {
margin-inline-start: auto;
}

&.pf-m-flex-grow#{$breakpoint-name} {
flex-grow: 1;
}

&.pf-m-align-self-#{$alignment}#{$breakpoint-name} {
align-self: #{$alignment};
}

Alignment on group, item, content, content-section

&.pf-m-align-items-#{$alignment}#{$breakpoint-name} {
align-items: #{$alignment};
}

Insets on toolbar, content, and content-section

.#{$toolbar},
.#{$toolbar}__content,
.#{$toolbar}__content-section {
// Inset modifiers
@each $inset, $inset-value in $pf-v6-c-toolbar--inset-map {
&.pf-m-inset-#{$inset}#{$breakpoint-name} {
--#{$toolbar}__content--PaddingInline: #{$inset-value};
}
}
}

Wrapping on group, item, content, content-section

&.pf-m-wrap#{$breakpoint-name} {
flex-wrap: wrap;
}

&.pf-m-nowrap#{$breakpoint-name} {
flex-wrap: nowrap;
}

Gaps on group, item, content, content-section

&.pf-m-gap-#{$gap}#{$breakpoint-name} {
row-gap: #{$gap-value};
column-gap: #{$gap-value};
}

&.pf-m-column-gap-#{$spacer}#{$breakpoint-name} {
column-gap: #{$spacer-value};
}

&.pf-m-row-gap-#{$spacer}#{$breakpoint-name} {
row-gap: #{$spacer-value};
}

Margins on group, item, content, content-section - remove these

&.pf-m-margin-inline-start#{$spacer}#{$breakpoint-name} {
margin-inline-start: #{$spacer-value};
}
&.pf-m-margin-inline-end#{$spacer}#{$breakpoint-name} {
margin-inline-end: #{$spacer-value};
}

Show/hide on group.pf-m-toggle-group

&.pf-m-show#{$breakpoint-name} {
.#{$toolbar}__group,
.#{$toolbar}__item {
display: flex;
flex: 0 1 auto;
}
.#{$toolbar}__toggle {
display: none;
}
}

&.pf-m-hide#{$breakpoint-name} {
.#{$toolbar}__group,
.#{$toolbar}__item {
display: none;
}
.#{$toolbar}__toggle {
display: inline-block;
}
}

@mattnolting
Copy link
Contributor

mattnolting commented Jun 21, 2024

Hidden/visible - supported on content, content-section, group, item, group.pf-m-label-group-container, group.pf-m-label-group

Why is it on .#{$toolbar}__group 3 times?

.#{$toolbar}__content-section,
.#{$toolbar}__group,
.#{$toolbar}__item,
.#{$toolbar}__group.pf-m-label-group-container,
.#{$toolbar}__group.pf-m-label-group {
@include pf-v6-hidden-visible(flex);


Hidden/visible update:

Duplicate selectors should be removed

.#{$toolbar}__group.pf-m-label-group-container
.#{$toolbar}__group.pf-m-label-group




.#{$toolbar}__content {
@include pf-v6-hidden-visible(grid);
}


Toolbar content display update:

Retain this. .pf-v6-c-toolbar__content houses pf-v6-c-toolbar__content-sections and could be used to hide/display whole sections of toolbar, especially in mobile presentation.




Width/min-width vars

.#{$toolbar}__item {
@include pf-v6-build-css-variable-stack("--#{$toolbar}__item--Width--base", "--#{$toolbar}__item--Width", $pf-v6-c-toolbar--breakpoint-map);
@include pf-v6-build-css-variable-stack("--#{$toolbar}__item--MinWidth--base", "--#{$toolbar}__item--MinWidth", $pf-v6-c-toolbar--breakpoint-map);


Min width update:

Due to the complexity and size of this feature, it can be simplified to width/min-width vars without responsive extensions.




Alignment on item and group

&.pf-m-align-start#{$breakpoint-name} {
margin-inline-start: 0;
}

&.pf-m-align-center#{$breakpoint-name} {
margin-inline-start: auto;
margin-inline-end: auto;
}

&.pf-m-align-end#{$breakpoint-name} {
margin-inline-start: auto;
}


Alignment update:

Alignment modifiers work as expected, we should retain them.

Screenshot 2024-06-21 at 12 04 31 PM Screenshot 2024-06-21 at 11 47 45 AM



&.pf-m-flex-grow#{$breakpoint-name} {
flex-grow: 1;
}


Flex grow update:

Flex grow modifier work as expected, we should retain it.

Screenshot 2024-06-21 at 12 06 16 PM



&.pf-m-align-self-#{$alignment}#{$breakpoint-name} {
align-self: #{$alignment};
}

Alignment on group, item, content, content-section


Alignment update:

Alignment should be isolated to group, item, and content-sections




&.pf-m-align-items-#{$alignment}#{$breakpoint-name} {
align-items: #{$alignment};
}

Insets on toolbar, content, and content-section

.#{$toolbar},
.#{$toolbar}__content,
.#{$toolbar}__content-section {
// Inset modifiers
@each $inset, $inset-value in $pf-v6-c-toolbar--inset-map {
&.pf-m-inset-#{$inset}#{$breakpoint-name} {
--#{$toolbar}__content--PaddingInline: #{$inset-value};
}
}
}


Inset update:

Insets can be isolated to content-section as gap is adjustable for content-section, group, and item.




Wrapping on group, item, content, content-section

&.pf-m-wrap#{$breakpoint-name} {
flex-wrap: wrap;
}

&.pf-m-nowrap#{$breakpoint-name} {
flex-wrap: nowrap;
}


Wrapping update:

Until we have a better approach to handling overflow, this is the simplest alternative as it allows overflowing groups and items to wrap. However, it should be isolated to content-section, group, and item.




Gaps on group, item, content, content-section

&.pf-m-gap-#{$gap}#{$breakpoint-name} {
row-gap: #{$gap-value};
column-gap: #{$gap-value};
}

&.pf-m-column-gap-#{$spacer}#{$breakpoint-name} {
column-gap: #{$spacer-value};
}

&.pf-m-row-gap-#{$spacer}#{$breakpoint-name} {
row-gap: #{$spacer-value};
}


Gap update:

Gap for content should remain consistent. Gap for group, item, content and content-section should remain adjustable.




Margins on group, item, content, content-section - remove these

&.pf-m-margin-inline-start#{$spacer}#{$breakpoint-name} {
margin-inline-start: #{$spacer-value};
}
&.pf-m-margin-inline-end#{$spacer}#{$breakpoint-name} {
margin-inline-end: #{$spacer-value};
}


Margin update:

Remove these.




Show/hide on group.pf-m-toggle-group

&.pf-m-show#{$breakpoint-name} {
.#{$toolbar}__group,
.#{$toolbar}__item {
display: flex;
flex: 0 1 auto;
}
.#{$toolbar}__toggle {
display: none;
}
}

&.pf-m-hide#{$breakpoint-name} {
.#{$toolbar}__group,
.#{$toolbar}__item {
display: none;
}
.#{$toolbar}__toggle {
display: inline-block;
}
}


Show/hide update:

Retain show/hide for resize observer functionality when available. This feature supports selector updates when available space is inadequate, without DOM manipulation. However, we might discuss updating show/hide to hidden/visible.




@patternfly-build
Copy link

🎉 This issue has been resolved in version 6.0.0-alpha.168 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nicolethoen nicolethoen moved this from In Progress to PR Review in PatternFly Issues Jul 1, 2024
@nicolethoen nicolethoen linked a pull request Jul 1, 2024 that will close this issue
@mattnolting
Copy link
Contributor

closed via #6838

@github-project-automation github-project-automation bot moved this from PR Review to Done in PatternFly Issues Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants