Customizer now uses React 16 context. #5701
Conversation
…c-react into customizer-fix # Conflicts: # apps/todo-app/package.json
| this.props.className !== newProps.className || | ||
| this.props.checked !== newProps.checked || | ||
| this.props.theme !== newProps.theme | ||
| ); |
There was a problem hiding this comment.
What if styles prop changes? I feel like having shouldComponentUpdate for such a simple component is code smell and we're checking most of the props now. I'm curious why it exists at all
| return ( | ||
| // tslint:disable-next-line:no-any | ||
| <ComposedComponent {...defaultProps} {...this.props as any} /> | ||
| <CustomizerContext.Consumer> |
There was a problem hiding this comment.
👍 I can steal this for createComponent 😄
cliffkoh
left a comment
cliffkoh
left a comment
There was a problem hiding this comment.
Will do a more thorough review on Monday
apps/todo-app/package.json
Outdated
| "react": ">=16.3.2-0 <17.0.0", | ||
| "react-dom": ">=16.3.2-0 <17.0.0", | ||
| "typescript": "2.8.4", | ||
| "typescript": "3.0.0-rc", |
|
Looks like I may be blocked on this getting published in |
|
I think you have created a separate PR to get a smaller subset of this PR in, while the rest of the changes are blocked on enzyme fixing a bug... I will close this then until you can actually make progress on it. |
…c-react into customizer-fix # Conflicts: # packages/utilities/src/Customizer.tsx
| scope: string, | ||
| context: TContext | ||
| ) => IStyleableComponentCombinedProps<TViewProps, TStyleSet, TTheme>; | ||
| CustomizerContext: React.Context<TContext>; |
| TViewProps, | ||
| TStyleSet, | ||
| TTheme | ||
| >; |
There was a problem hiding this comment.
I feel like this cast shouldn't be needed since you've defined a return type for getCustomizations in IStylingProviders...
3 participants
Addresses #5691 and #5413.
We were observing some peculiarities with theme changing. Addressed multiple issues here:
We now use react16 context rather than the deprecated context for customizer. We think there may have been issues in the old context model wrt pure components not propagating context changes to children, but now I'm not completely sure. Nevertheless, less deprecated api usage is good.
Check shouldComponentupdate was too restrictive and didn't consider mutations to theme or classname. Adjusting logic fixed some theming issues we saw.
When
loadThemeis called to apply a global theme, we were not observing mutations to this, so Customizers wouldn't re-evaluate and re-render. We now do observe global theme changes and rerender.Minor tweak to webpack config.
All customizer tests still pass, and DetailsList now respects
Microsoft Reviewers: Open in CodeFlow