feat(toolbar): implement container queries#8025
feat(toolbar): implement container queries#8025srambach wants to merge 10 commits intopatternfly:mainfrom
Conversation
WalkthroughAdds container-query support for the Toolbar: new container breakpoint variables and mixins, updates to hidden/visible utilities to accept a container name, extensive toolbar SCSS to opt into container contexts, example CSS and expanded documentation, plus a .gitignore entry. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview: https://pf-pr-8025.surge.sh A11y report: https://pf-pr-8025-a11y.surge.sh |
There was a problem hiding this comment.
Looks great! As we discussed offline:
- Increasing the specificity of the existing hidden/visible/toggle-group rules to add
.pf-v6-c-toolbar:not({container-class})could have some unwanted side effects, so it would be great if we can leave the specificity as it is - it would be fine to increase under.pf-m-containerif needed since that's an opt-in - Make sure any examples that show container queries have
isBetain the start of the hbs block - Run visual regressions just to make sure nothing else changed
I think it would probably be a good idea to run them against the react repo, too, since I think toggle group examples have markup changes based on breakpoints that the core examples won't show. If you want to take a stab, here's how I would do it:
- Start the react dev server with its core package pointing to your core directory with the toolbar cq branch checked out
- In backstop.config.js comment out the first line for
fullscreenRoutes. - Add a new
const fullscreenRoutes: [...]- the code for that is below - Change
baseUrlto the react server port (localhost:8002) - Run that and if everything looks like it rendered OK (not checking for toolbar bugs), accept those screenshots.
- Update the react repo to not point to your local core (just run a regular react dev server), and run the screenshot job again. That should give you a diff of the react toolbar examples with and without your branch changes.
That should be it? Here's the fullscreenRoutes array reference above
const fullscreenRoutes = [
'/components/toolbar/react/toolbar-items',
'/components/toolbar/react/with-adjusted-inset',
'/components/toolbar/react/sticky-toolbar',
'/components/toolbar/react/with-groups-of-items',
'/components/toolbar/react/background-color-variants',
'/components/toolbar/react/component-managed-toggle-groups',
'/components/toolbar/react/consumer-managed-toggle-groups',
'/components/toolbar/react/with-filters',
'/components/toolbar/react/with-custom-label-group-content',
'/components/toolbar/react/stacked-example',
'/components/toolbar/react/toolbar-content-wrapping',
'/components/toolbar/react/toolbar-group-spacers',
'/components/toolbar/react/toolbar-item-spacers',
'/components/toolbar/react-demos/console-log-viewer-toolbar-demo',
'/components/data-list/react-demos/expandable-control-in-toolbar',
'/components/table/react-demos/sticky-columns-and-header-with-toolbar'
];
srambach
force-pushed
the
7967-toolbar-container-query
branch
from
91597f6 to
3c55e92
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.gitignoresrc/patternfly/components/Toolbar/examples/Toolbar.csssrc/patternfly/components/Toolbar/examples/Toolbar.mdsrc/patternfly/components/Toolbar/toolbar.scsssrc/patternfly/sass-utilities/mixins.scsssrc/patternfly/sass-utilities/scss-variables.scss
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T15:25:08.994Z
Learnt from: srambach
Repo: patternfly/patternfly PR: 8030
File: src/patternfly/components/TreeView/tree-view.scss:441-445
Timestamp: 2025-12-17T15:25:08.994Z
Learning: In PatternFly TreeView component (src/patternfly/components/TreeView/tree-view.scss), the `.pf-m-disabled` modifier on list items affects only the visual appearance of node content (text and icons), but intentionally does not change the toggle button color. The toggle remains functional and visually distinct to indicate the item can still be expanded/collapsed.
Applied to files:
src/patternfly/components/Toolbar/toolbar.scss
🪛 markdownlint-cli2 (0.18.1)
src/patternfly/components/Toolbar/examples/Toolbar.md
560-560: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
561-561: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
561-561: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
622-622: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
737-737: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
741-741: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
759-759: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-upload
🔇 Additional comments (10)
.gitignore (1)
33-33: Clarify the purpose and content of.claudeartifacts.The addition of
.claudeto gitignore lacks context: what does this pattern match, when is it generated, and why must it be excluded from version control? Given that the PR explicitly credits AI-assisted tooling (Claude and Cursor), silently ignoring generated artifacts without documentation may obscure the provenance of code changes and complicate code review traceability.Consider documenting the artifact's purpose (e.g., as a comment in .gitignore or in the PR description) or storing it in a standard location with explicit handling.
src/patternfly/components/Toolbar/examples/Toolbar.css (1)
83-97: LGTM! Clear demo styles for container query examples.The resizable container and spacer styles effectively support the documentation examples. The 290px spacer width directly corresponds to the offset between viewport and container breakpoints, making the comparison examples intuitive.
src/patternfly/sass-utilities/scss-variables.scss (1)
81-96: LGTM! Well-structured container breakpoint variables.The container breakpoint variables correctly implement the 290px offset from viewport breakpoints. The naming convention is consistent with existing breakpoint variables, and the map structure mirrors
$pf-v6-global--breakpoint-name-mapfor symmetry.src/patternfly/sass-utilities/mixins.scss (1)
220-271: LGTM! Well-designed dual-mode hidden/visible mixin.The conditional branching cleanly separates container-query mode from media-query mode. Using the same class names (
pf-m-hidden-on-*,pf-m-visible-on-*) for both modes ensures a consistent API for consumers, with the behavior determined by whether a container context is established.src/patternfly/components/Toolbar/examples/Toolbar.md (1)
925-930: LGTM! Comprehensive modifier documentation.The new container modifier documentation clearly explains the purpose and breakpoint values for each variant. The table format is consistent with existing modifier documentation.
src/patternfly/components/Toolbar/toolbar.scss (5)
101-106: LGTM! Clean exclusion pattern for container-modified toolbars.Using
:where(:not([class*="pf-m-container"]))effectively prevents double-application of hidden-visible behavior when container queries are in use, while maintaining zero specificity impact.
200-225: LGTM! Well-structured container context initialization.The container query opt-in block:
- Establishes container context with
container-typeandcontainer-name- Sets sensible defaults for toggle groups (filters hidden, toggle visible)
- Applies container-aware
pf-v6-hidden-visiblemixinThis provides a clean foundation for container-based responsive behavior.
415-442: LGTM! Good refactor extracting toggle group behavior into a reusable mixin.Extracting the show/hide logic into
pf-v6-toolbar-toggle-group-breakpoint-behaviorreduces duplication and makes the code more maintainable.
352-360: Verify expandable content default container breakpoint.The selector
:where(.pf-m-container:not([class*="pf-m-container-"]))targets.pf-m-containerwithout an explicit breakpoint suffix, defaulting tolg. This aligns with the documentation stating that.pf-m-containerdefaults to thelgbreakpoint (702px).
529-554: Clarify the.pf-m-containerselector logic in toggle group container query rules.The selector on lines 536-537 applies container query rules for the current
$breakpointin each loop iteration. However, the plain.pf-m-containerselector (without the-{breakpoint}variant) is included in both selectors and will match in every iteration, generating rules for all breakpoints (sm, md, lg, xl, 2xl). The.pf-m-container-{breakpoint}variant, by contrast, only matches when$breakpointequals that variant in a given iteration, so.pf-m-container-mdonly receives md-specific rules.This differs from the
expandable-contentsection which explicitly excludes variant classes using:not([class*="pf-m-container-"])on the plain.pf-m-containerselector. Either the toggle group code should follow the same pattern for consistency, or it should be documented why this behavior is acceptable (e.g., intentionally supporting all breakpoint modifiers on the plain variant).
|
Rebased and used Regression tests run against a new baseline as created in #8039 look ok. Toolbar just shows new examples; others seem likely to be rendering. Let's recheck after the updated tests are merged. One to check is /components/masthead/html-demos/with-toggle-group-filters-expandable-content-expanded-mobile/ |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/patternfly/sass-utilities/mixins.scss (1)
60-81: Container query mixin implementation looks solid.The mixin correctly handles base cases, performs breakpoint lookups, and generates appropriate
@containerrules with optional named containers. The conditional logic for named vs. unnamed containers is well-structured.Note: A past review already identified trailing whitespace on line 68 after the
map-getstatement.
📜 Review details
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/patternfly/components/Toolbar/examples/Toolbar.mdsrc/patternfly/sass-utilities/mixins.scss
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
src/patternfly/components/Toolbar/examples/Toolbar.md
765-765: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-upload
🔇 Additional comments (2)
src/patternfly/sass-utilities/mixins.scss (1)
220-271: LGTM! Clean implementation of container query support.The mixin correctly extends the existing hidden-visible pattern to support container queries through an optional parameter. The mutually exclusive if/else logic (lines 242-269) ensures that either container queries or media queries are generated, but never both, preventing conflicts. The implementation maintains backward compatibility by defaulting to media query mode when
$container-nameis null.src/patternfly/components/Toolbar/examples/Toolbar.md (1)
931-936: LGTM! Complete documentation of container query modifiers.The modifier table accurately documents all container query breakpoints and their pixel values, consistent with the detailed breakpoint documentation provided earlier in the file (lines 617-624).
| ### Comparing container breakpoints | ||
| This example shows how different breakpoint modifiers affect when the toolbar expands. |
There was a problem hiding this comment.
Add blank line after heading.
Per Markdown formatting standards and static analysis, headings should be surrounded by blank lines. Add a blank line between the heading at line 765 and the description at line 766.
🔎 Proposed fix
### Comparing container breakpoints
+
This example shows how different breakpoint modifiers affect when the toolbar expands.🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
765-765: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In src/patternfly/components/Toolbar/examples/Toolbar.md around lines 765-766,
the heading "### Comparing container breakpoints" is not followed by a blank
line; add a single blank line between the heading and the description line "This
example shows how different breakpoint modifiers affect when the toolbar
expands." so the heading is separated by an empty line per Markdown formatting
rules.
There was a problem hiding this comment.
Hey @coderabbitai, we generally haven't put that space in. I want to stay consistent with what we have.
There was a problem hiding this comment.
@srambach, understood! I'll respect your project's existing markdown formatting conventions and won't flag this in future reviews.
✏️ Learnings added
Learnt from: srambach
Repo: patternfly/patternfly PR: 8025
File: src/patternfly/components/Toolbar/examples/Toolbar.md:765-766
Timestamp: 2026-01-02T16:11:59.444Z
Learning: In the PatternFly project (patternfly/patternfly repository), markdown documentation files in src/patternfly/components/*/examples/*.md do not use blank lines after headings. This is an intentional style choice for consistency across the documentation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Ran React visual diffs as recommended - just one artifact difference. Note for future reference - in addition to the instructions above, I had to change one additional line in backstop.config.js |
3 participants
This adds container breakpoints and creates another set of breakpoint modifiers based on a toolbar container for each of the existing (media based) breakpoint modifiers.
Fixes #7967
Implemented with support from Claude and Cursor.
Summary by CodeRabbit
New Features
Documentation
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.