-
Notifications
You must be signed in to change notification settings - Fork 187
WD-32254-Updated equal heights row and pattern to use 4 4 8 grid #5729
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
base: main
Are you sure you want to change the base?
WD-32254-Updated equal heights row and pattern to use 4 4 8 grid #5729
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request migrates the equal heights row pattern and component from the legacy 12-column grid system (4/6/12) to the new 8-column grid system (4/4/8). The changes update grid class names, SCSS grid variables, and refactor the template logic to use a new shared macro for better code reusability.
- Migrated from legacy grid classes (
.row,.col) to new grid classes (.grid-row,.grid-col) - Updated SCSS to use 8-column grid variables (
$grid-8-columns) instead of legacy 12-column variables - Extracted equal heights block rendering into a reusable shared macro
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
templates/_macros/vf_equal-heights.jinja |
Updated grid class names from legacy to 8-column grid, changed quote styles, and refactored to use new shared macro |
templates/_macros/shared/vf_equal-heights-block.jinja |
New shared macro containing extracted equal heights block rendering logic for reusability |
scss/_patterns_equal-height-row.scss |
Migrated grid variables from 12-column to 8-column system, updated calculations, added support for both new and legacy grid class selectors |
releases.yml |
Added release notes for version 4.40.0 documenting the grid migration |
package.json |
Bumped version from 4.39.0 to 4.40.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
petesfrench
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA'd on some different pages and looks good. Just left some minor comments in the code.
|
Thanks for adding linked title @immortalcodes , I'm adding +1! |
petesfrench
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Just a few comments:
- I found a visual regression with data spotlight pattern's 2-block variant. It is using equal heights row component under the hood.
- Please refactor
title_linktotitle_link_attrsobject instead of an href string, for consistency with the object forwarding pattern we have been using in other patterns. - Revert version to 4.39.0 in package.json and releases.yml (it hasn't been released yet - we can release after this PR)
- Some docs cleanup suggestions
- Prettier needs to be run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From Prettier in CI:
[warn] Code style issues found in the above file. Run Prettier with --write to fix.
Fix this with npx prettier -c -w .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes have caused a visual regression in the data spotlight pattern's 2-block variant - that pattern uses the equal height row component.
Before
After
| { | ||
| "name": "vanilla-framework", | ||
| "version": "4.39.0", | ||
| "version": "4.40.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v4.39.0 has yet to be released (we can release it after this merges), can you revert this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v4.39.0 has yet to be released (we can release it after this merges), can you move these notes into the 4.39.0 release entry
| {%- if title | length > 0 -%} | ||
| <p class="p-heading--{{ subtitle_heading_level }}"> | ||
| {%- if title_link | length > 0 -%} | ||
| <a href={{ title_link }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have tended to do this in newer patterns with link_attrs object, rather than an href string, so that other attributes of the link can be customized (for instance aria labels, classes, as necessary). See data spotlight for example. Can you update accordingly so this is consistent between patterns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case it can be title_link_attrs.
| // TODO implement with new 4/4/8 grid (.grid-row) | ||
| // TODO this may be a breaking change. we are applying new grid column counts to the old row class, in order to migrate the entire component to the new grid. | ||
| // This needs to be carefully investigated. It may require a major version update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO's and "This needs to be carefully investigated" can be removed now - but we should add a comment explaining that we intentinoally use grid-8 column counts even for row- classes for backwards compatibility while also noting potential problems with that (that you have determined are rare cases)
Done
Fixes [list issues/bugs if needed]
#5587
https://warthogs.atlassian.net/browse/WD-32254
QA
Check if PR is ready for release
If this PR contains Vanilla SCSS or macro code changes, it should contain the following changes to make sure it's ready for the release:
Feature 🎁,Breaking Change 💣,Bug 🐛,Documentation 📝,Maintenance 🔨.package.jsonshould be updated relative to the most recent release, following semver conventionScreenshots
[if relevant, include a screenshot or screen capture]