-
Notifications
You must be signed in to change notification settings - Fork 194
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
Lunarfusion/css 174 breadcrumbs tokens #1569
Conversation
🚀 Deployed on https://pr-1569--spectrum-css.netlify.app |
0c8580c
to
cd55e10
Compare
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.
No blockers, looks really good overall. nice work on this!
a couple of questions, opportunity to update a copyright year and a potentially out of scope accessibility concern.
|
||
/* text regular active item */ | ||
--spectrum-breadcrumbs-font-size-current: var(--spectrum-font-size-200); | ||
--spectrum-breadcrumbs-font-family-current: var(--spectrum-font-family-base); /* placeholder for --spectrum-font-family-default */ |
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.
Do we want to keep these placeholder comments in here?
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.
Yeah, these are placeholder stand-ins for tokens that are not functional.
When we have encountered a token that is absent and needs to be created immediately or that is non-functional (like --spectrum-font-family-default, which does not point to a true font stack), we create a stand-in token in components > tokens > custom-spectrum > custom-vars.css. These tokens then compile into components > tokens > dist > index.css so they can be used.
I am leaving the placeholder note to have a record of the original token that was assigned for this incase the original token is later repaired and the placeholder/stand-in token needs to be removed.
inline-size: 100%; | ||
} | ||
|
||
> .spectrum-ActionButton { |
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.
Not blocking, would you be able to explain or point to some docs for what > .
is doing?
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.
CSS selector, selects all .spectrum-ActionButton elements that are immediate children (first ancestor, not grandchildren) of the parent.
So this will select:
.spectrum-Breadcrumbs--compact
.spectrum-Breadcrumbs-item
.spectrum-ActionButton
But it will not select:
.spectrum-Breadcrumbs--compact
.spectrum-Breadcrumbs-item
.spectrum-ActionButton
.spectrum-ActionButton
This selector was already in the CSS, and I try to preserve as much existing CSS as possible as long as it still aligns with the designs and is reflective of up to date code standards.
@@ -0,0 +1,17 @@ | |||
/* | |||
Copyright 2022 Adobe. All rights reserved. |
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.
should go ahead and update these years?
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.
✨ Great catch, thank you! Updated.
@@ -0,0 +1,17 @@ | |||
/* | |||
Copyright 2022 Adobe. All rights reserved. |
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.
this year too
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 again!
@@ -57,27 +57,27 @@ examples: | |||
status: Verified | |||
markup: | | |||
<nav> | |||
<ul class="spectrum-Breadcrumbs spectrum-Breadcrumbs--sizeL"> | |||
<ul class="spectrum-Breadcrumbs"> | |||
<li class="spectrum-Breadcrumbs-item"> | |||
<button class="spectrum-ActionButton spectrum-ActionButton--sizeM spectrum-ActionButton--quiet"> |
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.
accessibility concern: potentially out of scope since you did not edit these buttons
Having the SVG as the only content of the button and setting aria-hidden"true" results in a button that is empty to screen readers, which I was able to reproduce on Voiceover. Potential solution would be to add a title for the SVG.
Relevant to all open folder svg buttons. I tried to comment each but might of missed a couple.
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.
🎣 Good catch. I started a slack conversation with our Accessibility folks. @jnurthen
@@ -169,27 +169,27 @@ examples: | |||
status: Verified | |||
markup: | | |||
<nav> | |||
<ul class="spectrum-Breadcrumbs spectrum-Breadcrumbs--sizeS spectrum-Breadcrumbs--multiline"> | |||
<ul class="spectrum-Breadcrumbs spectrum-Breadcrumbs--multiline"> | |||
<li class="spectrum-Breadcrumbs-item"> | |||
<button class="spectrum-ActionButton spectrum-ActionButton--sizeM spectrum-ActionButton--quiet"> | |||
<svg class="spectrum-Icon spectrum-Icon--sizeM spectrum-ActionButton-icon spectrum-Breadcrumbs-folder" focusable="false" aria-hidden="true"> |
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.
same accessibility comment from line 63
@@ -258,27 +258,27 @@ examples: | |||
status: Verified | |||
markup: | | |||
<nav> | |||
<ul class="spectrum-Breadcrumbs spectrum-Breadcrumbs--sizeM"> | |||
<ul class="spectrum-Breadcrumbs spectrum-Breadcrumbs--compact"> | |||
<li class="spectrum-Breadcrumbs-item"> | |||
<button class="spectrum-ActionButton spectrum-ActionButton--sizeM spectrum-ActionButton--quiet"> |
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.
same accessibility comment from line 63
find: /--spectrum-breadcrumb(.*)-s-/; | ||
filter: color; | ||
replace: --spectrum-breadcrumb$1-; | ||
[dir="rtl"] & { |
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.
ohh fancy! nice.
Rebase to main
d1dd470
to
cdb9e00
Compare
Description
BREAKING CHANGE: This migrates the Breadcrumbs component to core tokens.
Note: as of 01/04, waiting for decision from design regarding the background color on draggable / focused breadcrumb items.
How and where has this been tested?
Screenshots
To-do list