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

Fix grid offsets #4741

Merged
merged 12 commits into from
Apr 21, 2023
Merged

Fix grid offsets #4741

merged 12 commits into from
Apr 21, 2023

Conversation

ClementChaumel
Copy link
Contributor

Done

Restored the prefix to the original value
explicitely set the target to .col in the common grid patterns

@webteam-app
Copy link

Demo starting at https://vanilla-framework-4741.demos.haus

@bartaz
Copy link
Contributor

bartaz commented Apr 20, 2023

@ClementChaumel small thing - we have this .grid-demo styling that makes columns redish to make them visible in example, this stopped working now as I guess it relies on having "grid prefix" in the classname.

image

I guess we need to extend the .grid-demo to work with .col as well.

@bartaz bartaz added Priority: High Should be addressed within current iteration Review: QA -1 and removed Review: QA needed labels Apr 20, 2023
@ClementChaumel
Copy link
Contributor Author

@ClementChaumel small thing - we have this .grid-demo styling that makes columns redish to make them visible in example, this stopped working now as I guess it relies on having "grid prefix" in the classname.

image

I guess we need to extend the .grid-demo to work with .col as well.

Ah yes nice catch!

@ClementChaumel
Copy link
Contributor Author

It should be fixed, PTAL @bartaz

scss/_patterns_grid.scss Outdated Show resolved Hide resolved
ClementChaumel and others added 4 commits April 21, 2023 09:49
Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>
Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>
Co-authored-by: Bartek Szopka <83575+bartaz@users.noreply.github.com>
Copy link
Contributor

@bartaz bartaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some SCSS warnings to fix

scss/_patterns_grid.scss Outdated Show resolved Hide resolved
scss/_patterns_grid.scss Outdated Show resolved Hide resolved
scss/_patterns_grid.scss Outdated Show resolved Hide resolved
scss/_patterns_grid.scss Outdated Show resolved Hide resolved
&:nth-child(2) {
@include vf-grid-column(calc($grid-columns / 4 * 3));
// if there is only one column we offset it to the right
&:only-of-type {
Copy link
Contributor

@bartaz bartaz Apr 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed that, this is pretty cool!

I'm not super sure it's clear that it does that, but before we have any better implementation of the offset we can keep that. I like the simplicity of it, even if not super explicitly clear that this is gonna work this way. Especially that no other variants of this row do this. But maybe they can?

Copy link
Contributor

@bartaz bartaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@bartaz bartaz merged commit 155c4ce into canonical:main Apr 21, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 Priority: High Should be addressed within current iteration Review: Code +1 Review: QA +1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants