Refactor(@visx/text): rewrite Text with hook #946
Conversation
|
@williaster Hi, I have rewrite Text by hook, please review it. Thanks |
Pull Request Test Coverage Report for Build 400037954
💛 - Coveralls |
|
@williaster Hi Chris, Did you got chance to review my code? :) |
|
Sorry for the delay @susiwen8 , was gone on holiday last week and should be able to review today 👀 👍 |
There was a problem hiding this comment.
Thanks for the PR @susiwen8 ! It's looking great overall, I left a first round of comments and can take another look after that! 👀
|
|
||
| const transform = transforms.length > 0 ? transforms.join(' ') : ''; | ||
|
|
||
| return [wordsByLines, startDy, transform]; |
There was a problem hiding this comment.
since this hook has >2 return values, I'm wondering if we should return an object with named values instead. This shouldn't affect memoization of the values at all.
|
@williaster Hi, any thoughts? |
There was a problem hiding this comment.
Hey @susiwen this is looking GREAT 😎 Thanks for simplifying the logic per my previous comments 👌 .
I had one more small comment for a variable name, I'm also going to pull the branch to verify it functionally locally but then we should be able to get it in!
| ).toBe('title test'); | ||
| shallow(<Label title="title test" resizeObserverPolyfill={ResizeObserver} />) | ||
| .find('visx-annotationlabel') | ||
| .forEach(item => { |
There was a problem hiding this comment.
can you explain why this needed to be changed? in theory the API of Text shouldn't change with these changes right?
a .forEach is dangerous because if there are no matches for visx-annotationlabel, there would be no assertions and the test would "pass". so if this was needed we should add expect.hasAssertions() to the test.
There was a problem hiding this comment.
@williaster That was little embarrassing, cause I'm not familiar with enzyme.... I have update label test case inspired by axis
https://github.com/airbnb/visx/blob/master/packages/visx-axis/test/Axis.test.tsx#L177
|
Functionally looks good to me 👍 🎉 Performance-wise seems similar or even slightly better (we should have CI checks for this ideally :) |
There was a problem hiding this comment.
Thanks again @susiwen8 this is awesome 💯
This should land in 1.3.0 which we can get out early next week unless you need it for something sooner 👍
|
@williaster Thanks for guiding. I really learn something!😁 |
🚀 Enhancements
add
@visx/textuseTexthook🏠 Internal
rewrite
@visx/textTextwithuseTexthook