Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify TextEditorComponent updateClassList to always add managed classes to element #17702

Merged
merged 2 commits into from Jul 19, 2018

Conversation

Projects
None yet
3 participants
@rholinshead
Copy link
Member

rholinshead commented Jul 17, 2018

Description of the Change

Currently, the updateClassList method on the TextEditorComponent does not re-add the managed classes (editor, mini, is-focused) whenever the <atom-text-editor> element is re-rendered with a changed class. This PR fixes the issue by always adding the newClassList classes to the element, relying on element.classList.add to determine if the classes already exist (and should be ignored).

Alternate Designs

N/A

Why Should This Be In Core?

This address an issue that exists in one of the main core classes, which is used within other atom/atom classes. Implementing the change in a separate package would add an unnecessary additional dependency

Benefits

This change will allow external packages to dynamically set element classes without having to worry about losing the managed classes (editor, mini, is-focused). This is especially useful in React components that reuse an element when re-rendering

Possible Drawbacks

This change will affect packages' styling of atom-text-editor elements which rely on the managed classes not being re-applied. In such cases, the packages can regain specificity by adding the appropriate managed class names to the selectors.

Verification Process

  • Added new spec test and verified passing tests
  • Verified by modifying the find-and-replace package's project-find-view to set the findEditor's class based on the text (see video), and ensuring the managed classes remain on the element
  • Manually tested elements with changes to verify correct functionality

Screencasts:

Before:

updateclasslist_before

After:

updateclasslist_after

rholinshead added some commits Jul 13, 2018

Currently, the updateClassList function on the TextEditorComponent do…
…es not properly re-add its managed classes (editor, is-focused, mini) to the element when the element has been re-rendered with changed classes passed in. This fixes the issue by always adding the newClassList classes to the element and relying on the element.classList.add to determine if the classes already exist (and should be ignored)

Released under CC0
Add spec to test that updateClassList does not blow away the managed …
…class names (editor, is-focused, mini) when the element class names are changed.

Released under CC0

@rholinshead rholinshead self-assigned this Jul 17, 2018

@rholinshead rholinshead requested review from nathansobo and as-cii Jul 17, 2018

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Jul 17, 2018

@maxbrunsfeld Can you take a look at this?

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented Jul 19, 2018

This seems fine. DOMTokenList.add is already a noop if the provided class is already present.

@maxbrunsfeld maxbrunsfeld merged commit e500f93 into master Jul 19, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the fb-rh-text-editor-component-updateClassList branch Jul 19, 2018

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented Jul 19, 2018

Thanks @rholinshead!

@rholinshead

This comment has been minimized.

Copy link
Member Author

rholinshead commented Jul 19, 2018

My pleasure. Thanks for the quick review and merge @maxbrunsfeld @lee-dohm!

facebook-github-bot added a commit to facebookarchive/nuclide that referenced this pull request Dec 6, 2018

Remove `updatePatchList` atom TextEditor Patching
Summary: We now require 1.31.1, and [a fix for this](atom/atom#17702) was shipped in 1.30.

Reviewed By: captbaritone

Differential Revision: D13324204

fbshipit-source-id: d102b2bebf1327f62609865a1c044c24123069d4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.