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: [M3-8617] - Autocomplete Popper re-rendering causing test failures #10978

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Sep 20, 2024

Description 📝

How to test 🧪

  • Verify the Phone Number select on http://localhost:3000/profile/auth does not break and that the styles look correct
  • Verify Autocompletes throughout the app work as expected

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @bnussman-akamai! 🙏

I'm really confident in this implementation: plan-selection.spec.ts passes 50/50 times, tests in core/linodes are passing, and I'm not seeing any regressions in any of the Autocompletes I've looked at in the app.

If you're open to it, we can take this out of draft and make this the PR for M3-8617, and I can update #10976 to contain only the M3-8602 fixes (the before -> beforeEach change in plan-selection.spec.ts). Or we can include the spec changes here and just close #10976 altogether

I really appreciate you taking a closer look and proposing this solution! Thanks again Banks!

@bnussman-akamai
Copy link
Member Author

we can take this out of draft and make this the PR for M3-8617, and I can update #10976 to contain only the M3-8602 fixes

This sounds good to me! I'll update this PR to reflect M3-8617 @jdamore-linode

@bnussman-akamai bnussman-akamai changed the title poc: Autocomplete Popover fix fix: [M3-8617] - Autocomplete Popover re-rendering Sep 20, 2024
@bnussman-akamai bnussman-akamai changed the title fix: [M3-8617] - Autocomplete Popover re-rendering fix: [M3-8617] - Autocomplete Popover re-rendering causing test failures Sep 20, 2024
@bnussman-akamai bnussman-akamai marked this pull request as ready for review September 20, 2024 14:20
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner September 20, 2024 14:20
@bnussman-akamai bnussman-akamai requested review from dwiley-akamai and carrillo-erik and removed request for a team September 20, 2024 14:20
@bnussman-akamai bnussman-akamai added Testing Bug Fixes for regressions or bugs and removed Proof of Concept labels Sep 20, 2024
@bnussman-akamai bnussman-akamai changed the title fix: [M3-8617] - Autocomplete Popover re-rendering causing test failures fix: [M3-8617] - Autocomplete Popper re-rendering causing test failures Sep 20, 2024
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Phone Number select & Autocomplete fields in Cloud look good ✅
Code review ✅

@bnussman-akamai bnussman-akamai merged commit 5b7c237 into linode:develop Sep 20, 2024
19 of 20 checks passed
@carrillo-erik
Copy link
Contributor

@bnussman Very nice! I seriously didn't understand the power of the slotProps and this helps see their usage. This refactor threw me for a spin. Since we are customizing the Autocomplete so much. I had started to look into other libraries for a phone number input component with a country flag. However, it felt redundant to introduce a new dependency when the point of the tix was to remove the react-select dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fixes for regressions or bugs Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants