feat(toolbar): implement container queries - #8025
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds opt-in container-query responsiveness for Toolbars through new Sass breakpoints and mixins, container-aware visibility and expandable-content rules, toggle-group handling, example styles, documentation, and a broader ChangesToolbar container-query responsiveness
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Toolbar
participant SassUtilities
participant Container
Toolbar->>Container: establish named inline-size context
Toolbar->>SassUtilities: apply container breakpoint visibility rules
SassUtilities-->>Toolbar: generate container-query styles
Container-->>Toolbar: determine responsive content and toggle display
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
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'
];
91597f6 to
3c55e92
Compare
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).
|
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 |
|
This PR has been automatically marked as stale because it has not had activity in the last 60 days. |
8f85249 to
840da26
Compare
|
This PR has been automatically marked as stale because it has not had activity in the last 60 days. |
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
.claudedirectory.