feat(col): modernizing Grid component #30658
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
bot
added
package: core
...c/components/grid/test/basic/grid.e2e.ts-snapshots/grid-basic-md-ltr-Mobile-Chrome-linux.png
Outdated
Show resolved
Hide resolved
This reverts commit 35903c0.
| @include margin-horizontal(var(--margin-calc), 0); | ||
| } | ||
|
|
||
| $grid-col-number: 12; |
There was a problem hiding this comment.
This caps generated classes at 12, but --ion-grid-columns is configurable. If somebody sets a higher number of columns in --ion-grid-columns, then their width will be calculated for that number of columns, but the number of generated columns will be incorrect.
Either change how you get this ceiling so it comes from the configured variable, or document that spans/orders/offsets > 12 require custom CSS.
There was a problem hiding this comment.
The configuration --ion-grid-columns will be set after the framework is compiled, and while is being used by the developer. And what you're mentioning will be handled by the future Ionic modular approach as talked with @brandyscarney - hence this was left like this. Let me know, if I should leave any special comment regarding the matter.
There was a problem hiding this comment.
Cool! It might be worth adding a comment to document that then, currently it just looks like a possibly-unintended issue and somebody reviewing this in the future might not have this context.
| configs({ modes: ['md'] }).forEach(({ title, screenshot, config }) => { | ||
| test.describe(title('grid: offsets'), () => { | ||
| test('should not have visual regressions', async ({ page }) => { | ||
| await page.goto(`/src/components/grid/test/offsets`, config); |
There was a problem hiding this comment.
Is this the right path? It's kind of odd that this test exists under offsets-pull-push, but is testing against offsets. Maybe this test should be in the offsets e2e and there should be tests here for offsets-pull-push? Or maybe you just accidentally used the wrong path? Not sure.
There was a problem hiding this comment.
It was a lapse. These tests for now will have show case the deprecated features Pull & Push, not being applied, as they are deprecated. It might be useful for the future. If you think otherwise, we can remove it completely.
| display: flex; | ||
|
|
||
| flex-wrap: wrap; | ||
|
|
||
| gap: var(--ion-grid-gap, 0px); |
There was a problem hiding this comment.
This will always be 0px, I believe, since you're setting this var and using it in the same block. Is this intended? Seems redundant.
Co-authored-by: Shane <[email protected]>
Co-authored-by: Shane <[email protected]>
Co-authored-by: Shane <[email protected]>
Co-authored-by: Shane <[email protected]>
What is the current behavior?
The existing grid component (
ion-grid,ion-row, andion-col) in Ionic was developed several years ago and has not received significant updates since then. As a result, it does not leverage modern CSS features. For example, the gutter (spacing) between columns is implemented using the border property, which is an outdated technique.What is the new behavior?
--ion-grid-gap: this new CSS variable, will indicate the gap size in the grid. Defaults to0px- the current value.ion-col: has a new way to calculate the width of the column. Additionally a new propertyorder(and responsive variants) was added, and will control the order of the column.ion-row: uses the newly introduced custom property--ion-grid-gap. This property will indicate the gap size in the grid.Does this introduce a breaking change?
The properties
pullandpushfromion-col, have been removed. The functionality achieved with them, is now achieved with the new propertyorderand the existingsize. More information and migration examples can be read inBREAKING.mdfile.Other information