Skip to content

fix: vertical rule doesn't render #476

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.

Sign up for GitHub

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

Jump to bottom
Merged
jianliao merged 4 commits into from
Jan 22, 2020
Merged

fix: vertical rule doesn't render #476

jianliao merged 4 commits into from
Jan 22, 2020

Conversation

@lazd
Copy link
Member

@lazd lazd commented Jan 14, 2020

Description

This PR does a few different things:

How and where has this been tested?

  • How this was tested: A little scrolly scroll
  • Browser(s) and OS(s) this was tested with: Chrome on macOS

Screenshots

image

To-do list

  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • This pull request is ready to merge.

@jianliao
Copy link
Contributor

jianliao commented Jan 15, 2020 •
edited
Loading

Hi @lazd, I ran vr test for this PR: https://spectrum-visual-regression.ci.corp.adobe.com/job/spectrum-css-vr-test-asset/158/artifact/spectrum-css/backstop_data/html_report/index.html

I noticed that some components become 1px higher. Since this PR has no font update as @bernhard-adobe 's #408, but both include the DNA 5.23 upgrade. Do you know what might cause this change ?

Reference:
image
Test:
image

@bernhard-adobe
Copy link
Contributor

bernhard-adobe commented Jan 15, 2020

excellent finding @jianliao this might explain some of those changes in #408.

@lazd
Copy link
Member Author

lazd commented Jan 21, 2020

@bernhard-adobe can you look into what changed and figure out if we need to change anything in DNA to avoid the shifts?

@bernhard-adobe
Copy link
Contributor

bernhard-adobe commented Jan 22, 2020

@lazd @jianliao and I spend last week 1.5 days researching what causes that change and we were unable to figure out what exactly might cause this half-ish pixel change. We inspected various elements for CSS changes and only saw that the typeface changed. We are assuming this is happening because of the font-weight changes and font-face changes coming from the DNA typography updates.
Feel free to let us know if you find a value that causes directly this change. Maybe something in the root level of the page.

@jianliao
Copy link
Contributor

jianliao commented Jan 22, 2020

I suggest that we should have a separated PR for upgrade DNA next time. We did spend some time isolate the problem.

@lazd
Copy link
Member Author

lazd commented Jan 22, 2020

@jianliao @bernhard-adobe understood. In the mean time, I will back out the DNA change and re-define the variables in the :root so this change does not include the vertical shift. Thanks!

@lazd lazd force-pushed the issue/455-vertical-rule branch from acd5c24 to b8fc887 Compare January 22, 2020 18:01
lazd
lazd commented Jan 22, 2020
View reviewed changes
components/rule/package.json
},
"devDependencies": {
"@spectrum-css/button": "^2.0.3",
"@spectrum-css/buttongroup": "^2.0.3",
Copy link
Member Author

@lazd lazd Jan 22, 2020

Choose a reason for hiding this comment

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

This dependency was missing before.

@lazd lazd force-pushed the issue/455-vertical-rule branch from 6335413 to ea7b85f Compare January 22, 2020 18:12
lazd
lazd commented Jan 22, 2020
View reviewed changes
components/rule/metadata/rule.yml
</svg>
</button>
</div>
<button class="spectrum-ActionButton spectrum-ActionButton--quiet">
Copy link
Member Author

@lazd lazd Jan 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Switched to ActionButton because Tool is deprecated. They are visually identical.

lazd
lazd commented Jan 22, 2020
View reviewed changes
components/rule/index.css
--spectrum-divider-vertical-height: 100%;

/* Work around DNA misspelling */
--spectrum-divider-small-vertical-width: var(--spectrum-divider-small-vertical-width, var(--spectrum-divider-small-veritcal-width));
Copy link
Member Author

@lazd lazd Jan 22, 2020

Choose a reason for hiding this comment

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

The current version of DNA has this token, but it's spelled wrong var(--spectrum-divider-small-veritcal-width). This works around that issue, and should work properly once the misspelled token is removed.

lazd
lazd commented Jan 22, 2020
View reviewed changes
components/rule/index.css
border-width: var(--spectrum-rule-medium-height);
border-radius: var(--spectrum-rule-medium-height);
border-width: var(--spectrum-divider-medium-height);
border-radius: var(--spectrum-divider-medium-height);
Copy link
Member Author

@lazd lazd Jan 22, 2020

Choose a reason for hiding this comment

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

All tokens renamed from -rule to -divider since the component was renamed in DNA. The previous tokens are still there, but are missing the new vertical-width tokens.

lazd
lazd commented Jan 22, 2020
View reviewed changes
components/rule/index.css
@import '../commons/index.css';

:root {
--spectrum-divider-vertical-height: 100%;
Copy link
Member Author

@lazd lazd Jan 22, 2020

Choose a reason for hiding this comment

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

Defining height as a variable fixes #455

lazd
lazd commented Jan 22, 2020
View reviewed changes
components/buttongroup/index.css
> .spectrum-Rule--vertical {
height: auto;
align-self: stretch;
}
Copy link
Member Author

@lazd lazd Jan 22, 2020

Choose a reason for hiding this comment

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

This makes it so Rules inside of ButtonGroups look nice.

@lazd
Copy link
Member Author

lazd commented Jan 22, 2020

I backed out the DNA changes, included a workaround for the misspelled token name, and this is ready to merge.

@jianliao
Copy link
Contributor

jianliao commented Jan 22, 2020

https://spectrum-visual-regression.ci.corp.adobe.com/job/spectrum-css-vr-test-asset/165/artifact/spectrum-css/backstop_data/html_report/index.html

The only mismatch is expected.

@jianliao jianliao merged commit cd48593 into master Jan 22, 2020
@lazd lazd mentioned this pull request Jan 24, 2020
Merged
4 tasks
@GarthDB GarthDB deleted the issue/455-vertical-rule branch September 30, 2021 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jianliao jianliao jianliao approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Vertical Divider (Rule) doesn't render Divider (Rule) vertical DNA tokens

4 participants

@lazd @jianliao @bernhard-adobe