feat: wrap all components with withStyles HOC #749

jbadan · 2019-09-26T21:39:41Z

Description

The new withStyles higher order component wraps each component and dynamically requires the relevant styles, or disable the styles based on props.

Pass custom styling via require statement in customStyles prop.
Disable all styles using disableStyles prop

This pr introduces storybook! 📖 🎉

Functional components now with wrapped with forwardRefs

npm run start:playground or npm run storybook to start it

NOTE that you will need to refresh the page to see different style combos because the style will persist across pages.

To do:

docs site - markdown for code samples not working. This has to do with the forwarding refs
bug: Numbers in the calendar not showing up in storybook but are in docs site
update all tests that use .state() - not compatible with wrapped components 💀 😭
audit subcomponents to see if they need withStyles wrapper
finish remaining components
figure out why docs site in netlify is bringing in the customTestStyles 😆 - hot pink navigation anyone?
localizationEditor cascade disableCSS
prop description tables

…to feat/withStyles

… Import and Properties to account for it

…react into feat/withStyles

…to feat/withStyles

greg-a-smith · 2019-10-01T16:24:26Z

src/ActionBar/ActionBar.js


-const ActionBar = ({ children, className, ...props }) => {
+const ActionBar = React.forwardRef(({ children, className, customStyles, disableStyles, ...props }, ref) => {


I think we can get rid of the customStyles in this prop pruning use case since this never gets passed here from withStyles. It should stay in the PropTypes definition, however, because consumers are unaware that the component is wrapped in an HOC.

greg-a-smith · 2019-10-01T17:02:00Z

src/TimePicker/TimePicker.js

@@ -3,6 +3,7 @@ import Popover from '../Popover/Popover';
 import PropTypes from 'prop-types';
 import Time from '../Time/Time';
 import TimePickerItem from './_TimePickerItem';
+import withStyles from '../utils/WithStyles/WithStyles';


Does TimePicker need the ref forwarding?

greg-a-smith · 2019-10-01T17:03:16Z

src/Toggle/Toggle.js

 import classnames from 'classnames';
 import FormItem from '../Forms/FormItem';
 import FormLabel from '../Forms/FormLabel';
 import PropTypes from 'prop-types';
 import React from 'react';
 import { TOGGLE_SIZES } from '../utils/constants';
-
+import withStyles from '../utils/WithStyles/WithStyles';
 class Toggle extends React.Component {


Does Toggle need the ref forwarding?

greg-a-smith · 2019-10-01T17:04:02Z

src/TreeView/TreeView.js

 import PropTypes from 'prop-types';
 import Tree from './_Tree';
 import TreeBranch from './_TreeBranch';
 import TreeCol from './_TreeCol';
 import TreeHead from './_TreeHead';
 import TreeItem from './_TreeItem';
 import TreeRow from './_TreeRow';
+import withStyles from '../utils/WithStyles/WithStyles';
 import React, { Component } from 'react';

 class TreeView extends Component {


Does TreeView need the ref forwarding?

greg-a-smith

Disregard my last 3 comments about ref forwarding -- I didn't notice they weren't functional components. All aboard!!! 🚢

markpalfreeman · 2019-10-01T15:17:54Z

src/ActionBar/ActionBar.js

 };

 ActionBar.Actions = ActionBarActions;
 ActionBar.Back = ActionBarBack;
 ActionBar.Header = ActionBarHeader;

-export default ActionBar;
+export { ActionBar as __ActionBar };


What is the purpose of these __ renamed exports?

To have access to the "plain" component for the purposes of building the docs site (property tables and import statements).

markpalfreeman · 2019-10-01T16:32:04Z

src/ActionBar/ActionBar.js

-        <div {...props} className={actionBarClasses}>{children}</div>
+        <div {...props}
+            className={actionBarClasses}
+            ref={ref}>


Why was this ref (and similar others) added? Usually they're for focus management, but this is just a containing div

The reasoning was to go ahead and add them to every functional component being wrapped in withStyles as it will be a breaking change to add them going forward. See: https://reactjs.org/docs/forwarding-refs.html#note-for-component-library-maintainers

markpalfreeman · 2019-10-01T16:59:09Z

src/Alert/Alert.js

@@ -64,12 +65,13 @@ class Alert extends Component {
                        )}
                        <div className='fd-alert__text'>
                            {type && (
-                                <Icon glyph={`mesage-${type}`} />


Is this mesage a typo?

markpalfreeman · 2019-10-01T17:10:54Z

src/DatePicker/DatePicker.test.js

@@ -23,7 +23,7 @@ describe('<DatePicker />', () => {
    });

    test('open/close by calendar icon button', () => {
-        wrapper = mount(defaultDatePicker);
+        wrapper = mount(defaultDatePicker).children().children();


There are ~80 instances of this .children().children() pattern—can we write a single mount function with a comment for why we need to dive down 2 levels?

Definitely should be on the to do for the next iteration ✅

markpalfreeman · 2019-10-01T17:23:05Z

src/utils/WithStyles/WithStyles.js

+    };
+
+    //HOCs do not automatically pass refs as they are not technically props
+    function forwardRef(props, ref) {


This function name makes it sound like an imperative side-effecty thing, when really this is a component. Is there a better name?

Also, can this just return the "with ref" version so the hoisted thing below is a little clearer? Maybe something like:

const componentWithRef = (props, ref) => React.forwardRef(<WithStyles {...props} forwardedRef={ref} />); const componentWithStaticMethods = hoistNonReactStatic(componentWithRef, WrappedComponent);

We can definitely try that out in the next iteration - I took this from the react docs to rather be safe than sorry.

jbadan and others added 30 commits September 23, 2019 14:18

wip

29ce1e2

hoisting AND forward refs woooo

aa8d552

forwardRefs + fundamental styles bump

c7e1054

wip

071c855

input group

0442ee1

FormFieldset

bc574df

FormFieldset

1ae2b0f

FormInput

6cb2ec5

FormItem

0396207

FormLabel

0be093a

FormLegend

1e79a46

formMessage

878d2f5

fix build script

df97fd0

move WithStyles into directory

dcfd8eb

FormSelect with edit to FormFieldset capitalization

fc7f58b

FormSet

084ccbb

FormTextArea

3c36108

withStyles tests

67d7f6e

remove uneeded describe block

912255a

cleanup withstyles testing

767612f

Merge branch 'feat/withStyles' of github.com:SAP/fundamental-react in…

bcdaa27

…to feat/withStyles

reformat disableCSS check

ad5c2fe

Icon, Image, Identifier

cc4c05b

InlineHelp

b410d44

more components

1493df7

merge conflict resolved

93e9bbd

Layout Grid

15b3820

fix merge conflicts

af8f098

Link

6d2dda2

more components done

6e199ad

jbadan and others added 14 commits September 30, 2019 14:08

try ignoring file for netlify:

ce2e047

Merge branch 'feat/withStyles' of github.com:SAP/fundamental-react in…

df72ff0

…to feat/withStyles

move customStyles to storybook file

3fa70b1

ignore stories in build

6e132f5

remove space

75c4a9b

webpack config update

d3f72eb

revert changes

7513b29

try webpack ignore plugin for netlify

29b6710

Removed the withStyles from ActionBarBack

4b1b425

ignore stories

c4f2ee0

Cleanup of PropertyDescription

8fb1f7b

Added additional export from components using withStyles and adjusted…

e3781e9

… Import and Properties to account for it

Merge branch 'feat/withStyles' of https://github.com/SAP/fundamental-…

f3b369d

…react into feat/withStyles

excluse testcss

2c423cf

jbadan changed the title ~~feat: WIP: wrap all components with withStyles HOC~~ feat: wrap all components with withStyles HOC Oct 1, 2019

jbadan requested a review from a team October 1, 2019 15:04

greg-a-smith and others added 4 commits October 1, 2019 10:08

Added padding to the tile examples

7332d96

Style override for rendered markdown in property tables

d2b1200

bump fundamental styles to latest

1ac5bbd

Merge branch 'feat/withStyles' of github.com:SAP/fundamental-react in…

62c1bc9

…to feat/withStyles

greg-a-smith suggested changes Oct 1, 2019

View reviewed changes

remove customStyles from prop destrcturing

d1b8d8b

greg-a-smith reviewed Oct 1, 2019

View reviewed changes

greg-a-smith approved these changes Oct 1, 2019

View reviewed changes

bump react deps

d5cf717

jbadan merged commit f4f08d7 into master Oct 1, 2019

jbadan deleted the feat/withStyles branch October 1, 2019 17:23

markpalfreeman suggested changes Oct 1, 2019

View reviewed changes

jbadan mentioned this pull request Oct 2, 2019

fix: withStyles followup including typo fix and new test utility #761

Merged

jbadan mentioned this pull request Oct 25, 2019

expose ref for focus able element #576

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: wrap all components with withStyles HOC #749

feat: wrap all components with withStyles HOC #749

jbadan commented Sep 26, 2019 •

edited

Loading

greg-a-smith Oct 1, 2019

greg-a-smith Oct 1, 2019

greg-a-smith Oct 1, 2019

greg-a-smith Oct 1, 2019

greg-a-smith left a comment

markpalfreeman Oct 1, 2019

greg-a-smith Oct 1, 2019

markpalfreeman Oct 1, 2019

jbadan Oct 1, 2019

markpalfreeman Oct 1, 2019

markpalfreeman Oct 1, 2019

jbadan Oct 1, 2019

markpalfreeman Oct 1, 2019

jbadan Oct 1, 2019


		const ActionBar = ({ children, className, ...props }) => {
		const ActionBar = React.forwardRef(({ children, className, customStyles, disableStyles, ...props }, ref) => {

feat: wrap all components with withStyles HOC #749

feat: wrap all components with withStyles HOC #749

Conversation

jbadan commented Sep 26, 2019 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-a-smith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbadan commented Sep 26, 2019 •

edited

Loading