Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
🚀 New Component: SidePanel (Labs) #866
🚀 New Component: SidePanel (Labs) #866
Changes from 19 commits
c173fd6
1ab9273
7a0125a
a42345b
a9e88a5
6ae4407
ae32fbd
6a19f27
9690f43
b133b32
2b1c98a
cf84d3d
3f4222b
d5ca0f0
9d3881a
ac4863f
13b746e
0ad05da
67efbeb
fc2d4b7
5ad218a
dafa7cc
988348d
7f6052e
9a35375
9525c73
e6c76d7
ee30da8
86543f3
0f9fb80
33b6a8a
93a4d6c
0ddb7a1
ee501ed
1184430
767d8ba
fe62e14
011b172
8a50a60
ab71869
03eda56
42347dd
08eeea1
434f04e
6870461
e6c190d
3365528
2482039
8faa5c4
9caf331
d95ed68
a949f42
b2394ac
689e85a
69a30c8
6c21859
118c97a
748a80b
5859ac4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Question: Did we decide to use
CSSObject
for things like this, or useas const
?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 think what I was going for was to make sure that this object only had keys of the type
SidePanelVariant
(I also now know I can do this with theRecord
utility type). So in the future if we add a variant, we can make sure it can be accessed since we docontainerVariantStyle[variant]
later in the code.Not sure if
as const
really plays into this unless we're going for type inference on this object (then I'm not sure how to type it so that we ensure that the keys are only fromSidePanelVariant
).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 WD component team ran into an issue where
CSSProperties
(which we define asReact.CSSProperties & Emotion.CSSObject
) caused performance issues for Typescript compilation. I was able to observe the issue, but not reproduce it. Digging into the types ofCSSObject
, I found this:You can see that
CSSObject
is extending fromCSSOthersObjectForCSSObject
which expands all properties tostring
. This gives you autocomplete, but no type safety.margin
is a suggestion, butmargon
is a perfectly okay mistype.We think the typecheck slowdown comes from multiple levels of mapped types.
CSSObject
is about 900 properties which could cause slowdowns.as const
is a way to narrow values of objects from widened types likestring
to narrower types like'absolute'
. SoCSSObject
is useful for autocomplete, but will not save you from a mistyped property name.I asked the WD component team to create an issue with a reproduceable build that shows the slowdown so that we can directly address it. I have experienced periodic slowdowns in the Typescript language service, but I don't know what exactly is causing it. I wouldn't be surprised that mapped types of large interfaces causes performance issues.
A compromise might be to specify which CSSObject properties we intend to use: