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

Var mapping #4106

Merged
merged 4 commits into from
Oct 28, 2021
Merged

Conversation

lyubomir-popov
Copy link
Contributor

Done

Add list of variable remappings to the 3.0 upgrade guide.

QA: compare the values with the actual rem units, ensure they are matching between old/new vars.

@webteam-app
Copy link

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

@lyubomir-popov lyubomir-popov changed the base branch from main to vanilla-3.0 October 28, 2021 11:22
Copy link
Contributor

@anthonydillon anthonydillon left a comment

Choose a reason for hiding this comment

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

Few suggestions and a question in the code

templates/docs/upgrade-guide-v3.md Outdated Show resolved Hide resolved
| Spacing variable | Formula | Default value |
| --------------------- | ---------------------------- | ------------- |
| `$sph-inner--small` | `$sph--small` | `0.5rem` |
| `$sph-inner` | `$sph--large` | `1rem` |
Copy link
Contributor

Choose a reason for hiding this comment

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

if $sph--small and $sph--large are the same as $spv--small and $spv--large why not drop the concept of v and h and just have sp-large, sp-small, etc?

Copy link
Contributor

Choose a reason for hiding this comment

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

For vertical spacing we have much more options than for horizontal and we wanted to keep this distinction.

We need vertical spacing from x-small up to x-large, but we don't want people to use that much spacing horizontally between elements. So we kept separate sph and spv var families to better define which options are available for which type of spacing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @bartaz for summarizing this. Basically we want to keep track of what is being used vertically vs horizontally, and the prefix allows us to see at a glance that $sph-medium is not used or needed for example. So if someone finds themselves needing that value, they will be able to immediately tell that they are about to introduce something new to the framework and hopefully double check if it is indeed necessary, or whether an existing var might be a better choice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks that is clear 👍

templates/docs/upgrade-guide-v3.md Outdated Show resolved Hide resolved
templates/docs/upgrade-guide-v3.md Outdated Show resolved Hide resolved
lyubomir-popov and others added 2 commits October 28, 2021 13:24
Co-authored-by: Anthony Dillon <me@anthonydillon.com>
Co-authored-by: Anthony Dillon <me@anthonydillon.com>
@lyubomir-popov
Copy link
Contributor Author

@anthonydillon I've committed your suggestions, please have another look.

Co-authored-by: Anthony Dillon <me@anthonydillon.com>
Copy link
Contributor

@anthonydillon anthonydillon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@lyubomir-popov lyubomir-popov merged commit 58ad18e into canonical:vanilla-3.0 Oct 28, 2021
bartaz pushed a commit that referenced this pull request Nov 3, 2021
* update upgrade guide

* Update templates/docs/upgrade-guide-v3.md

Co-authored-by: Anthony Dillon <me@anthonydillon.com>

* Update templates/docs/upgrade-guide-v3.md

Co-authored-by: Anthony Dillon <me@anthonydillon.com>

* Update templates/docs/upgrade-guide-v3.md

Co-authored-by: Anthony Dillon <me@anthonydillon.com>

Co-authored-by: Anthony Dillon <me@anthonydillon.com>
bartaz pushed a commit that referenced this pull request Nov 3, 2021
* update upgrade guide

* Update templates/docs/upgrade-guide-v3.md

Co-authored-by: Anthony Dillon <me@anthonydillon.com>

* Update templates/docs/upgrade-guide-v3.md

Co-authored-by: Anthony Dillon <me@anthonydillon.com>

* Update templates/docs/upgrade-guide-v3.md

Co-authored-by: Anthony Dillon <me@anthonydillon.com>

Co-authored-by: Anthony Dillon <me@anthonydillon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants