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

fix(Dropdown|Modal|TextArea): update refs handlers #1360

Merged
merged 10 commits into from
Feb 22, 2017

Conversation

layershifter
Copy link
Member

Addresses #607.

@codecov-io
Copy link

codecov-io commented Feb 21, 2017

Codecov Report

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

@@            Coverage Diff             @@
##           master    #1360      +/-   ##
==========================================
+ Coverage   99.74%   99.74%   +<.01%     
==========================================
  Files         140      140              
  Lines        2345     2346       +1     
==========================================
+ Hits         2339     2340       +1     
  Misses          6        6
Impacted Files Coverage Δ
src/collections/Form/Form.js 100% <ø> (ø)
src/modules/Modal/Modal.js 100% <100%> (ø)
src/modules/Popup/Popup.js 100% <100%> (ø)
src/addons/TextArea/TextArea.js 100% <100%> (ø)
src/modules/Checkbox/Checkbox.js 100% <100%> (ø)
src/modules/Dimmer/Dimmer.js 100% <100%> (ø)
src/modules/Dropdown/Dropdown.js 100% <100%> (ø)

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 4dbaaf0...44c132c. Read the comment docs.

@@ -60,6 +60,8 @@ class TextArea extends Component {
this.updateHeight(e.target)
}

handleRef = c => (this.rootNode = c)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, we should also standardize these ref variable names. We currently use *Node, _* and *Ref in various places.

A ref can either be a node (DOM component) or a reference to a class instance (Composite component). Due to this, I vote we go with this.*Ref for clarity and accuracy.

Copy link
Member Author

@layershifter layershifter Feb 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it makes sense 👍

Do I have to change the names to these?

-handleRef = c => (this.rootNode = c)
+handleRef = c => (this.textAreaRef = c)
-handleSearchRef = c => (this._search = c)
+handleSearchRef = c => (this.searchRef = c)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right to my eyes 👍

@levithomason
Copy link
Member

levithomason commented Feb 22, 2017

I've updated the refs per our agreement. Refs at the root I simply used this.ref and this.handleRef for the naming convention. Mirrors how we handle onClick this.handleClick vs onItemClick this.handleItemClick.

Give it a look over and let me know if we're good to go here.

@layershifter
Copy link
Member Author

I've updated Checkbox handler. LGTM 👍

@levithomason levithomason merged commit ecf91c4 into master Feb 22, 2017
@levithomason levithomason deleted the fix/performance-updates branch February 22, 2017 22:39
harel pushed a commit to harel/Semantic-UI-React that referenced this pull request Feb 25, 2017
* fix(Dropdown|Modal|TextArea): update refs handlers

* fix(TextArea): update ref names

* fix(Form): update ref names

* fix(Checkbox): update ref names

* fix(Dimmer): update ref names

* fix(Dropdown): update ref names

* fix(Modal): update ref names

* fix(Popup): update ref names

* style(Checkbox): update ref handler
@levithomason
Copy link
Member

Released in semantic-ui-react@0.67.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants