feat(MountNode): add component #2407

layershifter · 2018-01-05T14:14:42Z

Core problem

Our Modal component makes too many things, the main problem is the management of classList on mountNode. It made too many problem for us, I thought about it and here is solution.

Solution

We need a declative way for classList management on mountNode, I've took a look on react-document-title and made similar component.

layershifter · 2018-01-05T14:22:06Z

gulp/tasks/docs.js

-  `${config.paths.src()}/collections/**/*.js`,
-  `${config.paths.src()}/modules/**/*.js`,
-  `${config.paths.src()}/views/**/*.js`,
+  `${config.paths.src()}/addons/*/*.js`,


I think that we need to store component specific functionality in its directory, this will allow to manage easier in future, see #1443.

Yep, looks like we already have a flat structure any how.

guillaumep · 2018-01-06T01:23:10Z

@layershifter Please consider the changes made to test/specs/modules/Modal/Modal-test.js in #2394. It fixes an invisible issue in the tests that were made visible by my proposed changes.

The issue is that the modals are never unmounted, thus modals mounted in one tests interacts with modals mounted in another test. This was provoking various seemingly unrelated modal tests to fails.

guillaumep · 2018-01-06T01:24:09Z

@layershifter Also, your approach to fix this issue looks promising!

levithomason · 2018-01-10T04:56:42Z

src/addons/MountNode/MountNode.js

+  shouldComponentUpdate({className: next}) {
+    const {className: current} = this.props
+
+    return next !== current


I know we've started this pattern of renaming destructured keys elsewhere but I'd prefer more explicit names like nextClassName and currClassName.

levithomason · 2018-01-10T05:18:54Z

src/addons/MountNode/lib/computeClasses.js

+  _.map(_.split(/\s+/)),
+  _.flatten,
+  _.uniq,
+)([...components])


I didn't test this, but I'm pretty sure:

Lodash will gracefully handle null/undefined args so no need to check if components exists first

Lodash will not mutate the original array so no need copy if first

There is a _.flatMap that will handle mapping and flattening at the same time.

This will currently include empty string classNames. However, switching the filter to an _.identity will filter all falsy values.

This can probably be simplified to:

const computeClasses = _.flow( _.map('props.className'), _.filter(_.identity), _.flatMap(_.split(/\s+/)), _.uniq, )

Which works like so:

computeClasses(null) //=> [] computeClasses(undefined) //=> [] computeClasses([ null, undefined, {}, { props: {} }, { props: { className: null } }, { props: { className: undefined } }, { props: { className: 'foo' } }, { props: { className: 'foo bar' } }, { props: { className: '0' } }, { props: { className: 'false' } }, ]) // => [ 'foo', 'bar', '0', 'false' ]

levithomason · 2018-01-10T05:22:20Z

src/modules/Modal/Modal.js

    }

+    if (this.state.mountClasses !== classes) newState.mountClasses = classes
+    console.log(classes)


levithomason · 2018-01-10T05:23:29Z

src/modules/Modal/Modal.js

    }

+    if (this.state.mountClasses !== classes) newState.mountClasses = classes
+    console.log(classes)
+    if (Object.keys(newState).length > 0) this.setState(newState)


For consistency, how about this?

if (!_.isEmpty(newState)) this.setState(newState)

levithomason · 2018-01-10T05:25:35Z

src/modules/Modal/Modal.js

@@ -296,6 +284,8 @@ class Modal extends Component {
    if (!childrenUtils.isNil(children)) {
      return (
        <ElementType {...rest} className={classes} style={{ marginTop, ...style }} ref={this.handleRef}>
+          <MountNode className={mountClasses} node={mountNode || document.body} />


Whoops, accessing document.body will break SSR. Looks like Modal has a method for this purpose, this.getMountNode().

levithomason · 2018-01-10T05:25:43Z

src/modules/Modal/Modal.js

@@ -304,6 +294,8 @@ class Modal extends Component {

    return (
      <ElementType {...rest} className={classes} style={{ marginTop, ...style }} ref={this.handleRef}>
+        <MountNode className={mountClasses} node={mountNode || document.body} />


See above SSR comment.

levithomason · 2018-01-10T05:27:39Z

src/addons/MountNode/lib/handleChange.js

+import _ from 'lodash'
+import computeClasses from './computeClasses'
+
+let prevClasses = []


This will be shared across all MountNode instances. If two MountNodes are on the page, they will need their own copy of prevClasses to diff against, correct?

This might belong on the actual MountNode class so it can be instance specific.

levithomason · 2018-01-10T05:28:59Z

Oh boy, just noticed this wasn't ready for review 🤦‍♂️ Well, hopefully, the comments are of some help and not too intrusive! Sorry about that...

layershifter · 2018-01-10T19:45:19Z

@levithomason your feedback is awesome as always, will back there soon 👍

…React into feat/body # Conflicts: # index.d.ts # src/index.js # src/lib/customPropTypes.js

codecov-io · 2018-01-13T15:03:41Z

Codecov Report

Merging #2407 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2407      +/-   ##
==========================================
+ Coverage   99.74%   99.74%   +<.01%     
==========================================
  Files         154      160       +6     
  Lines        2712     2745      +33     
==========================================
+ Hits         2705     2738      +33     
  Misses          7        7

Impacted Files	Coverage Δ
src/addons/MountNode/lib/computeClassNames.js	`100% <100%> (ø)`
src/addons/MountNode/lib/handleClassNamesChange.js	`100% <100%> (ø)`
...ddons/MountNode/lib/computeClassNamesDifference.js	`100% <100%> (ø)`
src/modules/Modal/Modal.js	`100% <100%> (ø)`	⬆️
src/addons/MountNode/MountNode.js	`100% <100%> (ø)`
src/addons/MountNode/lib/getNodeFromProps.js	`100% <100%> (ø)`
src/addons/MountNode/lib/NodeRegistry.js	`100% <100%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f26bef...64c6942. Read the comment docs.

layershifter · 2018-01-13T15:04:34Z

@levithomason now it's ready for review 😸

levithomason · 2018-02-04T21:47:38Z

src/addons/MountNode/lib/handleClassNamesChange.js

+import computeClassNames from './computeClassNames'
+import computeClassNamesDifference from './computeClassNamesDifference'
+
+const prevClassNames = new Map()


Should this not also be a property on each instance of a MountNode? Currently, this map is shared between all MountNode instances, over the life of the app. So as they mount/unmount, this same map will be reused.

Yes, it's a planned solution. We use a DOM node as the key in Map, so we will have the same classes for the same node.

Here is a simple example:

const div = document.createElement('div') const Tree = () => ( <> <MountNode className='foo' node={div} /> <MountNode className='bar' node={div} /> </> )

Because we have the same Map, we will have foo bar className on the div element after Tree will be mounted.

levithomason · 2018-02-04T21:48:10Z

Just one final question on the use of a global Map.

layershifter · 2018-02-18T08:50:38Z

@levithomason ping

levithomason · 2018-02-26T07:34:59Z

Released in semantic-ui-react@0.78.3.

* feat(MountNode): add component * restore Responsive test * test(Modal): fix test

layershifter force-pushed the feat/mount-node branch from 07a3686 to c4c3dec Compare January 5, 2018 14:15

layershifter commented Jan 5, 2018

View reviewed changes

This was referenced Jan 5, 2018

fix(Modal): scrolling disabled when modal unmounts #2394

Closed

fix(Modal): scrolling disabled with multiple modals bug #2255

Closed

levithomason reviewed Jan 10, 2018

View reviewed changes

feat(MountNode): add component

ed80f05

layershifter force-pushed the feat/mount-node branch from c4c3dec to ed80f05 Compare January 13, 2018 14:04

layershifter added 3 commits January 13, 2018 16:06

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

aa04309

…React into feat/body # Conflicts: # index.d.ts # src/index.js # src/lib/customPropTypes.js

restore Responsive test

68aa8ed

test(Modal): fix test

98d26c5

layershifter added enhancement ready for review labels Jan 13, 2018

levithomason reviewed Feb 4, 2018

View reviewed changes

layershifter added 2 commits February 12, 2018 22:19

Merge branch 'master' into feat/mount-node

a6ec0f6

Merge branch 'master' into feat/mount-node

64c6942

levithomason merged commit 0c5e2f9 into master Feb 18, 2018

levithomason deleted the feat/mount-node branch February 18, 2018 21:22

layershifter mentioned this pull request Feb 22, 2018

Multiple modals scrolling bug #1157

Closed

levithomason mentioned this pull request Feb 25, 2018

Multi modal scroll not working #2553

Closed

beaugunderson mentioned this pull request Mar 5, 2018

Modals with dimmer: false throw an exception in 0.78.3 #2604

Closed

Brantron pushed a commit to Brantron/Semantic-UI-React that referenced this pull request Mar 14, 2018

feat(MountNode): add component (Semantic-Org#2407)

f789f72

* feat(MountNode): add component * restore Responsive test * test(Modal): fix test

layershifter mentioned this pull request Aug 10, 2020

chore(MountNode): deprecate component #4027

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(MountNode): add component #2407

feat(MountNode): add component #2407

layershifter commented Jan 5, 2018 •

edited

Loading

layershifter Jan 5, 2018

levithomason Jan 10, 2018

guillaumep commented Jan 6, 2018 •

edited

Loading

guillaumep commented Jan 6, 2018

levithomason Jan 10, 2018

levithomason Jan 10, 2018 •

edited

Loading

levithomason Jan 10, 2018

levithomason Jan 10, 2018

levithomason Jan 10, 2018

levithomason Jan 10, 2018

levithomason Jan 10, 2018

levithomason commented Jan 10, 2018

layershifter commented Jan 10, 2018

codecov-io commented Jan 13, 2018 •

edited

Loading

layershifter commented Jan 13, 2018

levithomason Feb 4, 2018

layershifter Feb 12, 2018

levithomason commented Feb 4, 2018

layershifter commented Feb 18, 2018

levithomason commented Feb 26, 2018

feat(MountNode): add component #2407

feat(MountNode): add component #2407

Conversation

layershifter commented Jan 5, 2018 • edited Loading

Core problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guillaumep commented Jan 6, 2018 • edited Loading

guillaumep commented Jan 6, 2018

Choose a reason for hiding this comment

levithomason Jan 10, 2018 • edited Loading

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

levithomason commented Jan 10, 2018

layershifter commented Jan 10, 2018

codecov-io commented Jan 13, 2018 • edited Loading

Codecov Report

layershifter commented Jan 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Feb 4, 2018

layershifter commented Feb 18, 2018

levithomason commented Feb 26, 2018

layershifter commented Jan 5, 2018 •

edited

Loading

guillaumep commented Jan 6, 2018 •

edited

Loading

levithomason Jan 10, 2018 •

edited

Loading

codecov-io commented Jan 13, 2018 •

edited

Loading