Skip to content
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

fix(react-slider): Export internal CSS vars #33682

Merged

Conversation

ValentinaKozlova
Copy link
Contributor

@ValentinaKozlova ValentinaKozlova commented Jan 17, 2025

Partner's ask. Discussed it with @micahgodbolt

Previous Behavior

CSS variables were internal

New Behavior

Copy link

github-actions bot commented Jan 17, 2025

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: entire library
1.169 MB
292.874 kB
1.17 MB
292.988 kB
448 B
114 B
react-slider
Slider
37.546 kB
12.631 kB
37.994 kB
12.723 kB
448 B
92 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Button, FluentProvider & webLightTheme
69.236 kB
20.182 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
224.18 kB
64.859 kB
react-components
react-components: FluentProvider & webLightTheme
44.473 kB
14.597 kB
react-portal-compat
PortalCompatProvider
8.39 kB
2.64 kB
react-timepicker-compat
TimePicker
108.551 kB
36.094 kB
🤖 This report was generated against 1dfc83038c5ee3f4bff09947def9a0a05e9c2adc

Copy link

Pull request demo site: URL

@micahgodbolt
Copy link
Member

yeah, this works fine. my only last suggestion would be to write a story about customizing, which would also validate the work.

@ValentinaKozlova ValentinaKozlova marked this pull request as ready for review January 20, 2025 12:28
@ValentinaKozlova ValentinaKozlova requested a review from a team as a code owner January 20, 2025 12:28
Copy link
Contributor

@dmytrokirpa dmytrokirpa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might not want to expose all internal variables. For instance, sliderThumbPositionVar is a computed value derived from progressVar, so modifying it independently might not be logical.

However, I agree that color and size variables could be safely exposed.

Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to double check to make sure we are not breaking our public API contract.

can you confirm that these vars were supposed to be exposed since release ? and this is just fix to previous assumption ?

we should be very careful about exposing internal things ( they are called internal for some reason )

ty

Copy link
Member

@micahgodbolt micahgodbolt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm fine without the desctructuring if we find that its better for compilers.

@dmytrokirpa
Copy link
Contributor

yeah, this works fine. my only last suggestion would be to write a story about customizing, which would also validate the work.

In theory, that sounds great, but I'm not sure how it would work in practice since we don't usually make such examples for other components and it will be inconsistent. So, I'd suggest converting that story to a VR test (to validate the changes) and adding a new section, "Overriding FUI component tokens," to the Styling Components and referencing it from the component docs page.

@ValentinaKozlova ValentinaKozlova requested a review from a team as a code owner January 31, 2025 10:33
@ValentinaKozlova ValentinaKozlova merged commit ace3f40 into microsoft:master Jan 31, 2025
16 checks passed
@ValentinaKozlova ValentinaKozlova deleted the fix/slider-export-vars branch January 31, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: internal CSS variables aren't exported for Slider component
5 participants