Pickers: convert to CSS-in-JS Part 2.3(final) - Suggestions#7452
Merged
Vitalius1 merged 19 commits intoJan 10, 2019
Conversation
added 11 commits
December 18, 2018 19:43
Vitalius1
requested review from
Jahnp,
JasonGore,
KevinTCoughlin,
atneik,
cliffkoh and
joschect
December 20, 2018 23:40
|
Contributor
Author
|
@aftab-hassan can't wait to have the ability to click and see what files are causing this blocker... |
Contributor
Author
|
@joschect 🙄 feel free to review this last PR for converting the Suggestions any time. Will address the feedback when back from vacation. |
joschect
reviewed
Jan 1, 2019
added 2 commits
January 4, 2019 13:07
joschect
approved these changes
Jan 9, 2019
Contributor
|
🎉 Handy links: |
3 tasks
evlevy
pushed a commit
to evlevy/office-ui-fabric-react
that referenced
this pull request
Jan 11, 2019
…t#7452) * Suggestion start conversion * Adjust hover state * Font size to iconButton through styles * Fix import in test file * Suggestions conversion almost done * More changes * Fix tests, imports and renaming stuff * Rush change log * Remove scss file * Remove unnecessary scss file and exported const from example page * Bring back support for scss styles * Add snapshot test for CSS-in-JS styles * Add some comments for future reference * Bring back SCSS styles support for SuggestionsItems * Update documentation and add comments * Rename back some protected members of BasePicker for backward compatibility. * Remove unnecesary ternary * API file update
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
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.
Pull request checklist
$ npm run changeDescription of changes
This PR is the last in the series of converting pickers to use CSS-in-JS. as in the previous PRs, I moved again 🙄 some stuff around and of course, I, again 🙄 couldn't stop myself for a little bit of clean up and TSDocs addition. Changes made in this PR:
The biggest change and probably somewhat controversial is renaming some components. Before jumping and blasting me with request changes PR blockers 😠 , let me first make my case and hopefully my analysis is not wrong. The components renamed are the ones I needed to convert to CSS-in-JS (SuggestionsandSuggestionsItem). These are some generic components that expect a type argument in order for them to be used and preserve some type safety.https://github.com/OfficeDev/office-ui-fabric-react/blob/f2922d6242b952e8d76dea1a9a6d056c5f58f843/packages/office-ui-fabric-react/src/components/pickers/Suggestions/Suggestions.tsx#L54
https://github.com/OfficeDev/office-ui-fabric-react/blob/f2922d6242b952e8d76dea1a9a6d056c5f58f843/packages/office-ui-fabric-react/src/components/pickers/BasePicker.tsx#L59
In order to pass thatTto them, I had to make sure we can pass it to thestyledfunction all the way to that components props type. So my implementation of this dilemma was to create a function which required a type parameter when invoked and returned a functional component from thestyledHOC. To use each component now you have to do something likeconst StyledTypedComponent = styledComponent<T>();. Because these components are not intended to be used as standalone components and in fact be used only internally with anything which extendsBasePickerand then passing from it theTto ->Suggestions->SuggestionsItem.Alternatives to that would mean to keep the old implementation along with its SCSS files, plus my changes that would bring lots of code duplication and confusion (and probably bundle size increase).
There is always a chance that someone used these subComponents separately asFloatingPickerdid in itsSuggestionsCore, but that would require to build your entire picker from scratch (which FloatingPicker kinda does by 🤧 copy/pasting lots of stuff from BasePicker)https://github.com/OfficeDev/office-ui-fabric-react/blob/f2922d6242b952e8d76dea1a9a6d056c5f58f843/packages/office-ui-fabric-react/src/components/FloatingPicker/Suggestions/SuggestionsCore.tsx#L3
..., so I kinda feel not sure about this change and that is why I expect some feedback and maybe ideas on how to better do it for this scenario. I am very open to changes 😄 .
EDIT:
Had another look and decided not to risk with a breaking change and leave everything as is and just add support for both styling approaches. In case someone used these components individually and outside the context of
BasePicker, then he can still have.scssstyles applied. On the other side when someone is just using a picker which extends fromBasePickerwithout changing any of its internal subComponents, then the CSS-in-JS styles will be applied. To preserve theTpassed all the way to down to these subComponents I wrap them withstyledright before render and due to them being intended for internal use only, for now, I am not exporting these JS styled versions to the public API. Left some comments and hope when a rewrite of the pickers will be performed all these workarounds can be cleaned up.3. Cleaned up some example pages and removed unused
.scssfiles for them.P.S. 2960 lines of these changes are just a snapshot I added. So everyone is welcome for reviews. Don't hit the back button 😄
Microsoft Reviewers: Open in CodeFlow