Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(List): remove listRef from API #489

Merged
merged 2 commits into from
Nov 19, 2018
Merged

Conversation

kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Nov 19, 2018

TODO

  • update CHANGELOG

listRef prop of List component has lead to the problem described in #453 (and is a blocker for this PR to be merged).

Reasons to remove listRef

Proposed changes eliminate this prop from List API for the following reasons:

  • ref cannot be applied in cases when ElementType is evaluated into React.SFC component (this is, actually, what tests were warn about)
  • the assumption that it will be always a DOM node provided as a value to listRef expressed in these lines is wrong, as React ref may reference component instance (and will reference DOM element only for string components like ul, which luckily was the common case for List):
 /** Ref callback with the list DOM node. */	
listRef?: (node: HTMLElement) => void

How to cover cases where listRef was supposed to be used?

Cases that this listRef prop was aimed to cover now can be safely and easily covered with Ref component being exported from Stardust:

// client component code
render() {
  return (
    <Ref innerRef=(capturedListDomNode => this.listNode = capturedListDomNode)>
      <List ...>
    </Ref>
  )
}

No changes to the List API are necessary for that.

@codecov
Copy link

codecov bot commented Nov 19, 2018

Codecov Report

Merging #489 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #489      +/-   ##
=========================================
- Coverage   88.42%   88.4%   -0.02%     
=========================================
  Files          42      42              
  Lines        1451    1449       -2     
  Branches      211     186      -25     
=========================================
- Hits         1283    1281       -2     
  Misses        163     163              
  Partials        5       5
Impacted Files Coverage Δ
src/components/List/List.tsx 61.66% <ø> (-1.24%) ⬇️

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 df2a657...fea9b65. Read the comment docs.

@kuzhelov kuzhelov changed the title Remove listRef from List API fix(List): remove listRef from API Nov 19, 2018
@kuzhelov kuzhelov added 🧰 fix Introduces fix for broken behavior. 🚀 ready for review labels Nov 19, 2018
@kuzhelov kuzhelov merged commit 448aaac into master Nov 19, 2018
@layershifter layershifter deleted the fix/remove-listref-from-list branch November 19, 2018 13:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants