Skip to content

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

Merged
levithomason merged 10 commits intomasterfrom
fix/performance-updates
Feb 22, 2017
Merged

fix(Dropdown|Modal|TextArea): update refs handlers#1360
levithomason merged 10 commits intomasterfrom
fix/performance-updates

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.

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