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) O3-3350: Resolve infinite re-rendering of the dropdown component in the tablet viewport #326

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

samuelmale
Copy link
Member

@samuelmale samuelmale commented Jun 18, 2024

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

This PR fixes an issue with the dropdown component re-rendering infinitely on selecting an option in tablet view port. This issue stems from the "downshift" library consumed by Carbon and it seems to be affecting React v18. It was since resolved in downshift v8.2.4 and carbon.

Out of scope

I've also changed Carbon from a peer dependency to a normal dependency.

Screenshots

2024-06-18 02-55-53 2024-06-18 02_56_36

Related Issue

https://issues.openmrs.org/browse/O3-3350

Other

Copy link

Size Change: -248 kB (-19.68%) 🎉

Total Size: 1.01 MB

Filename Size Change
dist/371.js 0 B -71.5 kB (removed) 🏆
dist/465.js 0 B -182 kB (removed) 🏆
dist/998.js 0 B -15.6 kB (removed) 🏆
dist/main.js 343 kB +42.3 kB (+14.07%) ⚠️
ℹ️ View Unchanged
Filename Size Change
dist/184.js 11.2 kB 0 B
dist/195.js 76.6 kB 0 B
dist/225.js 2.57 kB 0 B
dist/29.js 163 kB 0 B
dist/300.js 709 B 0 B
dist/31.js 0 B -5.32 kB (removed) 🏆
dist/318.js 0 B -2.43 kB (removed) 🏆
dist/327.js 1.84 kB 0 B
dist/335.js 967 B 0 B
dist/353.js 3.02 kB 0 B
dist/41.js 3.36 kB 0 B
dist/422.js 6.79 kB 0 B
dist/436.js 0 B -759 B (removed) 🏆
dist/474.js 116 kB 0 B
dist/491.js 9.18 kB 0 B
dist/540.js 2.63 kB 0 B
dist/55.js 756 B -1.38 kB (-64.67%) 🏆
dist/635.js 14.3 kB 0 B
dist/70.js 482 B 0 B
dist/776.js 3.2 kB 0 B
dist/8.js 0 B -3.68 kB (removed) 🏆
dist/800.js 245 kB -49 B (-0.02%)
dist/979.js 0 B -6.85 kB (removed) 🏆
dist/99.js 690 B 0 B
dist/993.js 3.09 kB 0 B
dist/openmrs-form-engine-lib.js 3.72 kB -48 B (-1.27%)

compressed-size-action

@samuelmale samuelmale requested a review from arodidev June 19, 2024 10:27
Copy link
Contributor

@arodidev arodidev left a comment

Choose a reason for hiding this comment

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

Thanks @samuelmale

@arodidev arodidev merged commit 4ac3a45 into main Jun 19, 2024
4 checks passed
@arodidev arodidev deleted the fix/dropDownInfiniteRerenders branch June 19, 2024 11:07
@denniskigen
Copy link
Member

Just tested on dev3 and this fix works as intended! Thanks, @samuelmale.

@@ -29,6 +29,7 @@
"access": "public"
},
"dependencies": {
"@carbon/react": ">1.47.0 <1.50.0",
Copy link
Member

Choose a reason for hiding this comment

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

@samuelmale Is there a specific reason you made this a dependency and not a peerDependency?

I've read the linked issues/pr and the issue with the downshift dependency in carbon which has since been resolved. After pulling the latest changes and linking with the form-builder, i got some errors from carbon because of version conflicts.

  1. On one end adding this range resolved the issue with the downshift library but it also creates either a conflict or multiple versions of @carbon/react running within microfrontends that have dependencies out of that range for example esm-form-builder and other esms are currently running on @carbon/react@npm:1.37.0 which will in turn affect the bundle size. Would this necessitate us updating the frontends that pull the FE directly to at least 1.47.0 to have this fix working?
Screenshot 2024-06-21 at 11 02 10
  1. Doesn't having the same range of versions as a peer dependency yield the same result but with the benefit of not duplicating instances of @carbon/react?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC, the overwhelming consensus on the dependency management strategy for Carbon in O3 is having it as a normal dependency as opposed to a singleton (peer).

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

Successfully merging this pull request may close these issues.

4 participants