-
Notifications
You must be signed in to change notification settings - Fork 0
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
sara-component-updates-for-accounting #19
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…to sara-docsite-june
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.
Overall, really awesome. A few minor changes and questions.
@@ -80,7 +80,15 @@ console.log('Generating search index for documentation'); | |||
relativePath.startsWith('assets/') || | |||
relativePath.startsWith('dist/') || | |||
filename === '_sidebar.md' || | |||
filename === '404.md' | |||
filename === '404.md' || | |||
filename === 'border-radius.md' || |
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.
What's the rationale for excluding all these files below from the search?
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.
The rationale for hiding the border radius, color, elevation, spacing, and typography files is because I created new files that show our tokens & content (they're the ones linked in the sidebar under Styles), but you could still get to the orig Shoelace files through search & the information is actually not correct because it just shows the SL defaults. So I thought it'd be better to hide these from search so that people don't get to those page by accident & then mistakenly reference those values. The other hidden token files aren't really documented or used in Figma or our designs right now, so they're not technically incorrect but just not relevant yet, and since we can still get the same content from the Shoelace site (or even our own github repo, since the files are still there), I thought it'd be better to just hide them.
src/components/alert/alert.styles.ts
Outdated
@@ -20,60 +20,68 @@ export default css` | |||
border-top-width: calc(var(--sl-panel-border-width) * 3); | |||
border-radius: var(--sl-border-radius-medium); | |||
font-family: var(--sl-font-sans); | |||
font-size: var(--sl-font-size-small); | |||
font-size: var(--ts-font-sm); |
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.
I'm not sure we should be embedding references to --ts
vars directly into the library. We already have a layer that assigns the --sl-
vars to their corresponding --ts-
values; can we just use that? Otherwise we start introducing inconsistency.
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.
Ah, okay. I hadn't thought of that. I'll revert this & check other changes to make sure I've only used the --sl-
var
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.
So there are some font related decs where I previously created a --ts-
var and set a hard-coded value because there was no equivalent --sl-
var & I didn't want to update the --sl-
var. What do you think of leaving those as is? (--ts-leading-5
is one example. It's set in tokens.css
as 1.25rem
)
@@ -26,6 +26,7 @@ import type SlRemoveEvent from '../../events/sl-remove'; | |||
* @documentation https://shoelace.style/components/select | |||
* @status stable | |||
* @since 2.0 | |||
* @pattern stable |
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.
Is this also supposed to have a @figma
annotation?
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.
When there's no annotation, it defaults to just showing the "Figma Needed" tag, which is right for Select
-- right now we have no Figma component for Select.
@@ -100,7 +100,7 @@ export default css` | |||
|
|||
.tab-group--top .tab-group__indicator { | |||
bottom: calc(-1 * var(--track-width)); | |||
border-bottom: solid var(--track-width) var(--indicator-color); | |||
border-bottom: solid calc(4 * var(--track-width)) var(--indicator-color); |
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.
Slick
.tag--primary { | ||
background-color: var(--sl-color-primary-50); | ||
border-color: var(--sl-color-primary-200); | ||
.tag--blue { |
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.
I'm curious about why we're switching to presentational classes instead of semantic ones. Wouldn't that encourage people to use them in arbitrary places?
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.
The design team went back & forth on this a bit. We wanted to add three new decorative (non-semantic) colors, which seemed inconsistent to have alongside the existing 5 semantic naming for the variants, and we weren't sure that the primary
(blue) variant makes sense for us anyway, so we landed on just switching to presentational naming & just adding guidelines in Figma and the doc site (to do later) for generally reserving Yellow, Red, and Green for semantic uses. But after seeing that we're using the sl-tag
in a few places already I thought it wouldn't be a bad thing to keep the semantically named variants as well, so that you could use either danger
or red
to create a red tag. Open to your thoughts on how to update though, if you think this is a bad idea!
Fix syntax for boolean in slim example Co-authored-by: Michael Daross <daross@teamshares.com>
Fix boolean syntax for slim example Co-authored-by: Michael Daross <daross@teamshares.com>
Co-Authored-By: Michael Daross <daross@teamshares.com>
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.
Looking great!
Breadcrumb
,Tab
,Tag
Tag
, plus new variant names addedCard
andDetails
header
slot forAlert