-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat(tag): add "surface" intent #2279
Conversation
Add surface intent according to updated design specs #2243
} | ||
|
||
return ( | ||
<Tag key={intent} design={design} intent={intent as any}> |
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.
Sorry about the any
😅, but without it, TS will complain because it can't infer that due to my previous if
block, we cannot have a surface of intent, with a design of outlined or tinted when we end up 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.
You should be ashamed!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2279 +/- ##
=======================================
Coverage 97.56% 97.56%
=======================================
Files 760 760
Lines 5996 5996
Branches 2137 2100 -37
=======================================
Hits 5850 5850
Misses 144 144
Partials 2 2 ☔ View full report in Codecov by Sentry. |
type ValidTagDesignIntent = | ||
| FilteredDesignIntent<'tinted', 'surface'> | ||
| FilteredDesignIntent<'outlined', 'surface'> | ||
| FilteredDesignIntent<'filled'> |
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 is being done here is to prevent unwanted combinations.
The surface intent can only be applied if the design is set to filled.
<Tag design="tinted" intent="surface"> // ❌ TS will raise an error
<Tag design="outlined" intent="surface"> // ❌ TS will raise an error
<Tag design="filled" intent="surface"> // ✅
{ | ||
intent: 'surface', | ||
design: 'filled', | ||
class: tw(['bg-surface', 'text-on-surface']), |
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 still need this tw()
util?
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.
Unfortunately, yes. Otherwise, we lose the autocompletion / intellisense. This is due to a limitation of the Tailwind ESLint plugin's typings, which cannot handle very large objects, if I recall correctly
} | ||
|
||
return ( | ||
<Tag key={intent} design={design} intent={intent as any}> |
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.
You should be ashamed!
Wow that was quick, thx! Looks good to me |
TASK: #2243
Description, Motivation and Context
Add surface intent according to updated design specs
Types of changes