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

List: Convert to css-in-js #7133

Closed
KevinTCoughlin opened this issue Nov 16, 2018 · 5 comments
Closed

List: Convert to css-in-js #7133

KevinTCoughlin opened this issue Nov 16, 2018 · 5 comments

Comments

@KevinTCoughlin
Copy link
Member

KevinTCoughlin commented Nov 16, 2018

Describe the feature that you would like added

This feature is to convert [List] to use mergeStyles to take advantage of theming and styling.

At a glance, the only dynamic styling List does is:

https://github.com/OfficeDev/office-ui-fabric-react/blob/dbdbb23e7dac68b950388d9011108fa0cc2d5969/packages/office-ui-fabric-react/src/components/List/List.tsx#L435

The rest of the styling is simply adding global classNames to the List's DOM.

https://github.com/OfficeDev/office-ui-fabric-react/blob/dbdbb23e7dac68b950388d9011108fa0cc2d5969/packages/office-ui-fabric-react/src/components/List/List.tsx#L373

https://github.com/OfficeDev/office-ui-fabric-react/blob/dbdbb23e7dac68b950388d9011108fa0cc2d5969/packages/office-ui-fabric-react/src/components/List/List.tsx#L411

https://github.com/OfficeDev/office-ui-fabric-react/blob/dbdbb23e7dac68b950388d9011108fa0cc2d5969/packages/office-ui-fabric-react/src/components/List/List.tsx#L468

To Investigate

Prior to adding this feature we must confirm that refactoring the styling logic in List will:

  • Enable theming benefits for the control.
  • Not cause performance regressions for the control. This control is widely used and sensitive to changes which negatively impact performance.

cc: @JasonGore @bennettclark @micahgodbolt @natalieethell

@KevinTCoughlin
Copy link
Member Author

I can look into the performance implications of doing this work and report back to this thread. @JasonGore at a glance, does this feature seem necessary for fully supporting theming?

@JasonGore
Copy link
Member

JasonGore commented Nov 16, 2018

Given that List doesn't have any of its own styling and only one section, theming it directly may not be strictly necessary. @bennettclark Is it possible you could achieve your theming needs if List was simply wrapped by a themeable container?

Theming perf concerns are warranted and need to be addressed by #7003. If theming needs can be met by a wrapper and perf is a concern, it seems prudent to wait at least until #7003 is resolved.

@micahgodbolt
Copy link
Member

I'd 2nd the idea of not adding theming if we can avoid it. It's possible that people are using the list component on its own, and adding css in js library dependency would add a bit of bulk (like resizeGroup).

@JasonGore
Copy link
Member

JasonGore commented Nov 16, 2018

It seems like the need here wasn't to theme List directly, but to pass theme to List items which can be done using Customizer. I think we can table this work for now.

@KevinTCoughlin
Copy link
Member Author

It seems like the need here wasn't to theme List directly, but to pass theme to List items which can be done using Customizer. I think we can table this work for now.

Sounds good to me.

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants