-
Notifications
You must be signed in to change notification settings - Fork 671
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
Improve type error messages and tooltips #910
Conversation
- Interfaces support declaration merging, and thus are superior in this case. An user can add his own color names. The index signature might be removed some day for "strict mode". - The comment is outdated. - Smaller types in tooltips lead to more readable type errors.
@jxnblk Which colors in I'm making them all optional for now to avoid changing so many tests. |
94b7162
to
2bef788
Compare
@@ -373,14 +372,14 @@ test('warns when initialColorModeName matches a key in theme.colors.modes', () = | |||
}) | |||
|
|||
test('dot notation works with color modes', () => { | |||
const Button = (props) => { |
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.
damn, can't Prettier decide on one style? I'll revert this change.
This is starting to become pretty big, and while I didn't tackle all todos yet, it might be good to start reviewing before it becomes overwhelming. |
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 love this cleanup!! 🔥 So good 👍
Do you want to ship this as-is and work on the other fixes in another PR or keep this one around for a bit longer?
Generally speaking, none should be required, but the color mode functionality does depend on the For the interoperability part, I think we'll want to create a "blessed" Theme interface (along with some color palette interfaces) after the TS conversion is out as an optional piece of the puzzle, but not sure if that's helpful 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.
Looks generally good to me -- I'd say feel free to merge this whenever and continue to iterate with more PRs, if that makes working on this easier
@@ -1,11 +1,11 @@ | |||
import { SystemStyleObject } from '@theme-ui/css' | |||
import { ThemeUIStyleObject } from '@theme-ui/css' |
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 to bikeshed, but this is a much better name :)
Okay this might be a small thing, but I want to add some polish (lol) to the types.
Generally, the problem we have is that our types, mostly imported from DT, may be a bit confusing.
I'd like to make Theme UI types as welcoming as possible for v1 (#832)
Related issues:
merge
. UseTheme
as argument.Interpolation
and display errors onSystemStyleObject
better.csstype
and we actually have them insideStandardCSSProperties
, but they don't come tosx
prop. I can't see a reason we shouldn't support vendor properties on type level, while they work at runtime.CSS.VendorProperties<number | string>
toCSSProperties
type.label?: string
toThemeUIStyleObject
([@types/theme-ui] SxStyleProp & Emotion's label DefinitelyTyped/DefinitelyTyped#45020 (comment))SystemStyleObject
toThemeUIStyleObject
.Few notes to explain what I'm doing.
Intersections (
&
) turn a conflicting prop intonever
. This isn't something we often want.(TS Playground with example)