ResizeGroup SCSS to MergeStyles Part 2: Style Conversion #4072
Conversation
| data={ initialData } | ||
| onReduceData={ onReduceScalingData } | ||
| onRenderData={ onRenderData } | ||
| /> | ||
| ); | ||
| ).first().shallow().first().shallow(); |
There was a problem hiding this comment.
we should try to use that helper here, or, use mount.
| if (typeof root.type() === 'string') { | ||
| // If type() is a string then it's a DOM Node. | ||
| // If it were wrapped, it would be a React component. | ||
| throw new Error('Cannot unwrap this component because it is not wrapped'); |
There was a problem hiding this comment.
I think that we shouldn't throw in this case. I should be able to use this function anywhere that shallow can be used. If the TargetComponent is a div, and the root is a div then that's a fine case. If this should only be used in the case of a decorated component, then the name should reflect that. Something like shallowDecoratedComponent
| return root.first().shallow(shallowOptions); | ||
| } | ||
| // Unwrap the next component in the hierarchy. | ||
| root = root.first().shallow(shallowOptions); |
There was a problem hiding this comment.
While I don't think that any decorators we currently have render multiple siblings, it is possible that this will miss a component that isn't the first child.
| */ | ||
| export function shallowUntilTarget<P, S>( | ||
| componentInstance: React.ReactElement<P>, | ||
| TargetComponent: string, |
There was a problem hiding this comment.
You could have target component be type of Function, and then users could pass in the actual class that they care about. You could then access the string by calling TargetComponent.name and it would work out fine. That way it keeps things a bit more type safe since they would need to import the actual class they care about. This is just a suggestion, if you feel like trying it out that would be cool, otherwise no worries.
There was a problem hiding this comment.
This was the original way that I had implemented this, but specifically for decorated functions like ResizeGroupBase, the name becomes 'ComponentWithInjectedProps' so it returns before it should. Passing a string name into the function was a way to avoid this.
|
This is fine, but we might want to note that the component relies on some of those styles to function properly. Changing the visibility of the 'measured' div, or the position, will have really nasty side effects. Do we want to move those into the component rather than being considered "themed styles". |
|
@micahgodbolt That's a good point. Should we create a baseStyles pattern to use for essential styling properties? |
|
inline works fine for me. We should be making those styles hard to override as doing so might break the functionality. |
* master: ResizeGroup SCSS to MergeStyles Part 2: Style Conversion (microsoft#4072) Applying package updates. Import Unstyled Component for Layer, Nav, Image, ScrollablePane, ResizeGroup, and Rating (microsoft#4135) FocusZone: isDefaultPrevented is now respected (microsoft#4133) Website: Left nav scroll improvements (microsoft#4132) AutoFill: Component should also listen to onInput, just like the other input components do (microsoft#4129) GroupedList: Fixed chevron animation (microsoft#4123) CoachMarkStyles: Use ... instead of assign for IE compatibility" (microsoft#4130) Applying package updates.
) * convert scss to mergeStyles * rush change * updated snapshots * update experiments snapshots * temp fix for shallow test with decorator usage * added shallowUntilTarget function to Utilities for use with decorated components * cleanup * moved shallowUntilTarget into fabrics common folder * clean up imports * remove scss * added base to index * shallowUntilTarget default shallow functionality * removing base from index so styles are not optional * moved measured styles to inline * updated snapshots * missing semicolon * updated experiments snapshots
Pull request checklist
$ npm run changeDescription of changes
Convert all SCSS to MergeStyles in ResizeGroup component.