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.

Sign up for GitHub

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

Jump to bottom

WRQ-19325: Removed findDOMNode calls #3243

Merged
merged 29 commits into from
Jun 3, 2024
Merged

WRQ-19325: Removed findDOMNode calls #3243

merged 29 commits into from
Jun 3, 2024

Conversation

0x64
Copy link
Member

@0x64 0x64 commented May 30, 2024 •
edited by MikyungKim
Loading

Checklist

  • I have read and understand the contribution guide
  • A CHANGELOG entry is included
  • At least one test case is included for this feature or bug fix
  • Documentation was added or is not needed
  • This is an API breaking change

Issue Resolved / Feature Added

As findDOMNode will be removed in React 19, replace/remove findDOMNode in Enact.

Resolution

A new private internal HoC WithRef is added to provide an imperative handle to get a node ref based on a ref of a sibling dummy node. With this HoC, spotlight/Spottable, ui/Placeholder.PlaceholderControllerDecorator, and webos/speech.VoiceControlDecorator is updated not to call findDOMNode.
Also, ui/ResolutionDecorator is updated to use CSS classes with querySelector rather than ref.
ui/ViewManager is updated to add a dummy sibling node just like a new internal HoC. Since its implementation, HoC cannot be used directly.

Additional Considerations

Links

WRQ-19325
enactjs/sandstone/pull/1622
enactjs/agate/pull/792

Comments

0x64 added 24 commits May 8, 2024 14:50
@0x64
Update React to version 18.3.1
c35e9b5 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Removed findDOMNode from Spottable
f4e38f6 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Removed findDOMNode from ResolutionDecorator
3c35f99 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Refine resolution code
7d08305 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Update Spottable to use a siblling node rather than a wrapping node
a1e09c1 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Removed findDOMNode from View
1fd3945 
Note that some unit tests are failed so we need to check cases.

Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Implement WithRef private internal higher order component
eee5951 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Updated Spottable to use WithRef
56f123e 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Add guards to WithRef
5fe5583 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Updated unit tests for useSpottable but skip tests
8c3624a 
It is not unclear yet but it looks that Jest environment cannot handle a selector
containing semicolons. For example, :has() makes an error.

Due to this issue, WithRef's code cannot tested with Jest yet.

Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Fixed a wrong comment (found during work)
dbe161b 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Removed findDOMNode from VoiceControlDecorator
a122ac8 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Skipped unit tests for Spottable
170587e 
Also error occurs if a selector string contains semicolon

Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Update WithRef to cover both forwarded ref and a normal ref
381eec9 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Update Spottable to work with refs properly
cd15cab 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Fixed TransitionGroup to render properly when rerendered
e4bb9f5 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Removed findDOMNode from PlaceholderControllerDecorator
d01fa6e 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Updated unit tests along with changes of WithRef
655f76e 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Updated VoiceControlDecorator along with changes of WithRef
73954b3 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Removed findDOMNode from Scrollable/Scrollbar
f185af5 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Added unit tests for WithRef
ccfef09 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Update searching logic not to use :has() selector for unit tests
fbc74b7 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Update an unit test case to work properly
8b30724 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Merge branch 'develop' into feature/WRQ-19325
e5946ba
@codecov Codecov
Copy link

codecov bot commented May 30, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 95.83333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 82.48%. Comparing base (3cf15c2) to head (8dc71b2).

Files Patch % Lines
packages/core/internal/WithRef/WithRef.js 94.11% 1 Missing ⚠️
packages/ui/resolution/resolution.js 80.00% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #3243      +/-   ##
===========================================
+ Coverage    82.42%   82.48%   +0.06%     
===========================================
  Files          156      157       +1     
  Lines         7215     7247      +32     
  Branches      1912     1919       +7     
===========================================
+ Hits          5947     5978      +31     
- Misses         997      998       +1     
  Partials       271      271

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

0x64 added 2 commits May 31, 2024 17:59
@0x64
Revert "Removed findDOMNode from Scrollable/Scrollbar"
b0463fc 
This reverts commit f185af5.
@0x64
Updated changelogs
6ef862f 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
This was referenced Jun 3, 2024
Merged
Merged
@0x64
Updated changelogs
553c04e 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
juwonjeong
juwonjeong approved these changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@juwonjeong juwonjeong left a comment

Choose a reason for hiding this comment

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

LGTM

0x64 added 2 commits June 3, 2024 17:14
@0x64
Refined code as reviewed
50dc577 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
@0x64
Refined code as reviewed
8dc71b2 
Enact-DCO-1.0-Signed-off-by: Seungcheon Baek (sc.baek@lge.com)
MikyungKim
MikyungKim approved these changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@MikyungKim MikyungKim left a comment

Choose a reason for hiding this comment

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

LGTM

@MikyungKim MikyungKim merged commit 33c27d5 into develop Jun 3, 2024
7 checks passed
@MikyungKim MikyungKim deleted the feature/WRQ-19325 branch June 3, 2024 09:09
This was referenced Jun 4, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juwonjeong juwonjeong juwonjeong approved these changes

@MikyungKim MikyungKim MikyungKim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@0x64 @MikyungKim @juwonjeong