[added] Support using multiple document.body classes #452

indiesquidge · 2017-07-04T00:39:56Z

Fixes #451.

Changes proposed:

update bodyOpenClassName prop to handle adding and removing multiple class names
update String#includes polyfill to work properly
remove unmaintained and obsolete element-class library
add new helper for adding/removing classes from the body element
update refCount helper to have a getter and chainable methods

Use Case

I am using the Tachyons library for CSS styling in my app. If you have not worked with Tachyons before (it's wonderful), it is essentially a collection of atomic class names—i.e., class names that do only one thing (e.g. .fixed { position: fixed; }). Because of this, it is in the nature of Tachyons that multiple classes will be tacked on to a single element to achieve a desired style.

Recently, I ran into the issue outlined in #369. To solve the issue, I need to apply two CSS properties to the document.body element when the modal opens: position: fixed and overflow: hidden.

To do this with Tachyons, I need to have bodyOpenClassName='fixed overflow-hidden'.

However, this causes a bug in the current version of the react-modal library in which the classes are appended ad infinitum (see here).

Acceptance Checklist:

All commits have been squashed to one.
The commit message follows the guidelines in CONTRIBUTING.md.
Documentation (README.md) and examples have been updated as needed.
If this is a code change, a spec testing the functionality has been added.
If the commit message has [changed] or [removed], there is an upgrade path above.

indiesquidge · 2017-07-04T00:41:25Z

CONTRIBUTING.md

@@ -30,8 +30,6 @@ always be in sync.

 - `npm start` runs the dev server to run/develop examples
 - `npm test` will run the tests.
- `scripts/test` same as `npm test` but keeps karma running and watches


I removed this line because it seems like it's obsolete. When I run npm test, I karma keeps running and watches my changes.

This can be moved to another PR.

indiesquidge · 2017-07-04T00:44:37Z

docs/styles/classes.md

+simultaneously).
+
+`bodyOpenClassName` can support adding multiple classes to `document.body` when
+the modal is open. Add as many class names as you desire, delineated by spaces.


This section

bodyOpenClassName can support adding multiple classes to document.body when
the modal is open. Add as many class names as you desire, delineated by spaces.

is the only newly added documentation. The rest of the changes in this file were simply refactors of the existing documentation into a more easily editable form (e.g. line breaks at ~80 characters, newlines between separate paragraphs, removal of duplicate spacing, etc.)

If you'd prefer I undo these changes and merely add my documentation, just let me know.

coveralls · 2017-07-04T00:56:08Z

Coverage remained the same at 86.721% when pulling 2071476 on indiesquidge:support-multiple-body-classes into 581be77 on reactjs:master.

coveralls · 2017-07-04T00:56:08Z

Coverage remained the same at 86.721% when pulling 2071476 on indiesquidge:support-multiple-body-classes into 581be77 on reactjs:master.

diasbruno · 2017-07-04T01:57:21Z

Hi @indiesquidge, the reason for bodyOpenClassName is to customize the default ReactModal__Body--open and to provide an identifier or group identifier to know when to add or remove the class from document.body (in case of multiple modals opened - see refCount.js).

Initially, we made it a single constant class name, so we don't end up with a complex system to manage these classes on the refCount.

To accept this PR, this is the property it must hold:

Modal A: { open: true, bodyOpenClassName: A B }
Modal B: { open: true, bodyOpenClassName: A }

if Modal A is closed, A must be in classList.
if Modal B is closed, A B must be in classList.

If it fails on this, one shared class might disappear and it would be hard to find why.

The same result can be achieved using onAfterOpen and onRequestClose to inject and remove additional class names to the document.body.

diasbruno · 2017-07-04T02:04:31Z

CONTRIBUTING.md

@@ -30,8 +30,6 @@ always be in sync.

 - `npm start` runs the dev server to run/develop examples
 - `npm test` will run the tests.
- `scripts/test` same as `npm test` but keeps karma running and watches


This can be moved to another PR.

diasbruno · 2017-07-04T02:07:05Z

src/components/ModalPortal.js

-      elementClass(document.body).remove(bodyOpenClassName);
+      bodyOpenClassName.split(' ').forEach(className => {
+        document.body.classList.remove(className);
+      });


This might remove a shared class if more than one modal is opened.

indiesquidge · 2017-07-04T08:24:19Z

@diasbruno, thanks for the tips! I have updated my code to handle the test case you laid out about modals using shared classes.

the reason for bodyOpenClassName is to customize the default ReactModal__Body--open and to provide an identifier or group identifier to know when to add or remove the class from document.body

This makes sense for sure. I'm just hoping to be able to add multiple classes to document.body via the bodyOpenClassName prop, similar to how I can have a string of classnames for className and overlayClassName.

Initially, we made it a single constant class name, so we don't end up with a complex system to manage these classes on the refCount

I definitely agree that a complex system to manage the classes would be bad. I see that refCount is really just managing a mapping of classnames (keys) to a number of how many modals "added" each class (values). I took advantage of mapping in my update, with a few slight tweaks. Hopefully what I have implemented is not too complex.

The same result can be achieved using onAfterOpen and onRequestClose to inject and remove additional class names to the document.body.

Using these hooks is what I initially looked at doing, but I'm already doing a number of other things in each of the methods and figured having the ability to add multiple classes via the bodyOpenClassName prop would be worthwhile feature to have.

diasbruno · 2017-07-04T12:45:06Z

You can remove the artifacts (dist/*). It's generated when publishing the new version.
Later I'll finish to review this PR.

indiesquidge · 2017-07-04T18:32:17Z

Tests seem to be breaking on Node 6 & 7, but it works fine on my local machine (Node 8). Not sure what the issue is; I'll look into it later.

indiesquidge · 2017-07-04T19:57:26Z

Strange, even when I switch my node version to 6.11.0, all tests still pass on my machine...

diasbruno · 2017-07-04T21:59:18Z

Yeah, it seems to be working (I've download your branch and run the tests on node v8.1.2).
Note that travis-ci uses Firefox 38.0.0 to run the tests.

diasbruno · 2017-07-04T22:10:11Z

specs/Modal.spec.js

+    expect(isBodyWithReactModalOpenClass('A B')).toBe(false);
+    expect(isBodyWithReactModalOpenClass('A')).toBe(true);
+    unmountModal();
+    expect(isBodyWithReactModalOpenClass('A')).toBe(false);


Expectations should always be true.

expect(!isBodyWithReactModalOpenClass('A B')).toBeTruth(); expect(isBodyWithReactModalOpenClass('A')).toBeTruthy();

Sure, I can certainly change the the tests to use the toBeTruthy() method, but may I ask why it is preferred?

While working with these tests, the terseness of using toBeTruthy() on bang methods methods—instead of something like toBe(false)—makes it more difficult to assess what one is asserting.

Just to follow the current code style.

My opinion: I'd like to see something like expect(!something_that_is_false);, because every expectation ends up been a true statement.

F = F => T

diasbruno · 2017-07-04T22:11:07Z

src/components/ModalPortal.js

+    bodyOpenClassName
+      .split(' ')
+      .map(refCount.add)
+      .forEach(bodyClass => document.body.classList.add(bodyClass));


You can make this pipeline a function.

Something like this okay?

addClassToBody = (bodyClass) => { // Increment class(es) on refCount tracker and add class(es) to body bodyClass .split(' ') .map(refCount.add) .forEach(className => document.body.classList.add(className)); }

diasbruno · 2017-07-04T22:11:18Z

src/components/ModalPortal.js

+      .split(' ')
+      .map(refCount.remove)
+      .filter(bodyClass => classListMap[bodyClass] === 0)
+      .forEach(bodyClass => document.body.classList.remove(bodyClass));


You can make this pipeline a function.

Something like this okay?

removeClassFromBody = (bodyClass) => { const classListMap = refCount.get(); // Decrement class(es) from the refCount tracker // and remove unused class(es) from body bodyClass .split(' ') .map(refCount.remove) .filter(className => classListMap[className] === 0) .forEach(className => document.body.classList.remove(className)); }

I think you can create a new file on helpers folder.

Sweet! Any preference on the name? I was thinking something like bodyClassManager or bodyElementManager, but am definitely open to other suggestions.

Maybe remove and add are good enough, so we use with namespace.

// classname can be changed. import * as classname from '../helpers/classname'; classname.add(...); classname.remove(...);

indiesquidge · 2017-07-05T20:27:26Z

src/components/ModalPortal.js

    // Add body class
-    elementClass(document.body).add(bodyOpenClassName);
+    bodyClassList.add(bodyOpenClassName);


As this is iterated upon, it seems like we are moving in a direction very similar to the elementClass library that was here before. (The major under-the-hood difference being that bodyOpenClassName can be a string of multiple class names, and using the other helper refCount to keep track of opened modals relying on the class internally.)

If you would prefer, I could make bodyClassList look and feel similar to elementClass, i.e. have document.body be passed in as an argument like it is here. This would remove the line change here, thus keeping the "public API" of the helper class the same as how elementClass looked.

I've created a commit on another branch to demonstrate what this might look like.

I honestly prefer it to the more restrictive bodyClassList helper which is directly bound to the body element.

@diasbruno, which implementation would you prefer? Does the thumbs up mean you think I should implement the code outlined in the commit I linked to? I think the code in the external commit is a more robust solution than what currently exists in this PR, but it's up to you :)

ow sorry. the version that uses document.body directly. :)

indiesquidge · 2017-07-05T20:52:46Z

Note that travis-ci uses Firefox 38.0.0 to run the tests.

@diasbruno, is there any way to simulate this test run locally? Setting my node version to the same still passes for me locally; running the exact command Travis does (i.e. node_modules/.bin/cross-env NODE_ENV=test node_modules/.bin/karma start "--single-run") still passes for me locally.

Basically anything I can think of to try and "break" the test script locally by mimicking the Travis build is not working—the tests always seem to behave as I would expect when I run them locally.

diasbruno · 2017-07-05T21:46:11Z

I think this PR is ready. You can try installing Firefox 38 and run CONTINUOUS_INTEGRATION=true make tests-single-run or found out if there is some api on Firefox that can be incomplete or broken.

diasbruno · 2017-07-05T21:56:12Z

ping @ajfuller. Any thoughts?

fuller · 2017-07-06T01:20:55Z

Code looks nice to me 👍

I don't see anything that stands out as to why it's failing on Firefox 38 either. I'll try to pull down and test later, but here is the link to download Firefox 38:
https://ftp.mozilla.org/pub/firefox/releases/38.8.0esr/mac/en-US/

diasbruno · 2017-07-06T01:35:42Z

@indiesquidge I think you can push a commit with console.log on each step of the test to see what's going on.
Thanks @ajfuller.

* update `bodyOpenClassName` prop to handle adding and removing multiple class names * update String#includes polyfill to work properly * ensure shared classes on `document.body` persist on one modal close if multiple modals are open * create new helper for adding/removing class names from body element * remove unmaintained and obsolete `element-class` library * rename refCount private variable `modals` to `classListMap` * create `get` method on refCount helper for public access to the class list count

indiesquidge · 2017-07-06T18:59:54Z

@diasbruno, alright I finally got the test failing locally (thanks, @ajfuller for the link to Firefox 38).

I narrowed the issue down to String#includes. Firefox 38 does not have this function natively, so it relies on the polyfill provided in specs/helper.js. However, this polyfill doesn't appear to work correctly when given a string like 'A B' because it will split that string on the space and then check it against the unsplit string, which of course fails.

The polyfill breaks on other tests that should return true

String.prototype.includes = function(item) {
	return this.length > 0 && this.split(" ").indexOf(item) !== -1;
}

const str = 'To be, or not to be, that is the question.'
console.log(str.includes('To be')) // returns false; should be true

I have updated the polyfill to use the MDN polyfill, which seems to work properly and indeed the test passes after this change.

coveralls · 2017-07-06T19:03:05Z

Coverage increased (+0.5%) to 87.173% when pulling cfa9959 on indiesquidge:support-multiple-body-classes into 581be77 on reactjs:master.

coveralls · 2017-07-06T19:03:05Z

Coverage increased (+0.5%) to 87.173% when pulling cfa9959 on indiesquidge:support-multiple-body-classes into 581be77 on reactjs:master.

diasbruno · 2017-07-07T01:49:58Z

@indiesquidge This is great! Sorry for the long code review and thanks for the effort on this feature.

diasbruno · 2017-07-07T01:52:07Z

I'm merging this PR. Maybe later, we can think on how to remove the polyfill. Maybe we can run those tests with babel-polyfill (?) or something.

indiesquidge · 2017-07-09T18:48:55Z

Thanks @diasbruno for speedy replies and for helping me along with this PR. Glad I could help :)

diasbruno · 2017-07-11T17:21:03Z

Released v2.2.2.

indiesquidge commented Jul 4, 2017

View reviewed changes

diasbruno reviewed Jul 4, 2017

View reviewed changes

indiesquidge commented Jul 5, 2017

View reviewed changes

diasbruno merged commit 9076eb7 into reactjs:master Jul 7, 2017

indiesquidge deleted the support-multiple-body-classes branch July 9, 2017 18:47

[added] Support using multiple document.body classes #452

[added] Support using multiple document.body classes #452

Conversation

indiesquidge commented Jul 4, 2017 • edited Loading

Changes proposed:

Use Case

Acceptance Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 4, 2017

coveralls commented Jul 4, 2017

diasbruno commented Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indiesquidge commented Jul 4, 2017

diasbruno commented Jul 4, 2017

indiesquidge commented Jul 4, 2017

indiesquidge commented Jul 4, 2017

diasbruno commented Jul 4, 2017 • edited Loading

diasbruno Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

indiesquidge Jul 4, 2017 • 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

Choose a reason for hiding this comment

indiesquidge Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indiesquidge Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indiesquidge commented Jul 5, 2017

diasbruno commented Jul 5, 2017 • edited Loading

diasbruno commented Jul 5, 2017

fuller commented Jul 6, 2017

diasbruno commented Jul 6, 2017 • edited Loading

indiesquidge commented Jul 6, 2017

coveralls commented Jul 6, 2017

coveralls commented Jul 6, 2017

diasbruno commented Jul 7, 2017

diasbruno commented Jul 7, 2017 • edited Loading

indiesquidge commented Jul 9, 2017

diasbruno commented Jul 11, 2017

indiesquidge commented Jul 4, 2017 •

edited

Loading

diasbruno commented Jul 4, 2017 •

edited

Loading

diasbruno commented Jul 4, 2017 •

edited

Loading

diasbruno Jul 4, 2017 •

edited

Loading

indiesquidge Jul 4, 2017 •

edited

Loading

indiesquidge Jul 5, 2017 •

edited

Loading

indiesquidge Jul 5, 2017 •

edited

Loading

diasbruno commented Jul 5, 2017 •

edited

Loading

diasbruno commented Jul 6, 2017 •

edited

Loading

diasbruno commented Jul 7, 2017 •

edited

Loading