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

refactor: [M3-6901, M3-6909] - Replace Select with Autocomplete in: nodebalancers #10688

Conversation

harsh-akamai
Copy link
Contributor

Description 📝

We want to get rid of our dependency on react-select for accessibility reasons and to consolidate our usage of third-party libraries.

Changes 🔄

  • Replaced all instances of Select with Autocomplete in the NodeBalancer feature.

Target release date 🗓️

How to test 🧪

Verification steps

(How to verify changes)

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@harsh-akamai harsh-akamai added the NodeBalancers Relating to NodeBalancers label Jul 17, 2024
@harsh-akamai harsh-akamai self-assigned this Jul 17, 2024
@harsh-akamai harsh-akamai requested a review from a team as a code owner July 17, 2024 10:33
@harsh-akamai harsh-akamai requested review from dwiley-akamai and hkhalil-akamai and removed request for a team July 17, 2024 10:33
Copy link

github-actions bot commented Jul 17, 2024

Coverage Report:
Base Coverage: 82.5%
Current Coverage: 82.5%

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

You have 2 failing E2E tests:

  • open-support-ticket.spec.ts
    This is probably a flake and will pass upon re-run
  • smoke-create-nodebal.spec.ts
    This is likely a real failure caused by differences between Select and Autocomplete. See the test video below:
smoke-create-nodebal.spec.ts.mp4

@mjac0bs
Copy link
Contributor

mjac0bs commented Jul 17, 2024

You have 2 failing E2E tests:

  • open-support-ticket.spec.ts
    This is probably a flake and will pass upon re-run

Yeah, can confirm that this is flake. 😬 I hopefully fixed this in another PR, so disregard any failures on that one here.

… inline anonymous functions for onChange in AutoComplete : nodebalancers
…t on [Enter] for AutoComplete : nodebalancers
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for fixing test failures. Code review and manual testing looks good, just left one minor suggestion.

inputId={`type-${configIdx}`}
isClearable={false}
id={`type-${configIdx}`}
disableClearable={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit:

Suggested change
disableClearable={true}
disableClearable

@harsh-akamai harsh-akamai force-pushed the feature/M3-6909-replace-select-with-autocomplete-in-nodebalancers branch from ac64501 to c17e0b7 Compare July 20, 2024 06:52
@bnussman-akamai bnussman-akamai added the Approved Multiple approvals and ready to merge! label Jul 22, 2024
@harsh-akamai harsh-akamai merged commit dfe2e61 into linode:develop Jul 22, 2024
19 checks passed
@harsh-akamai harsh-akamai deleted the feature/M3-6909-replace-select-with-autocomplete-in-nodebalancers branch July 22, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! NodeBalancers Relating to NodeBalancers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants