Skip to content

refactor: [M3-9767] - Use simple Select in RegionTypeFilter - #12018

Merged
hkhalil-akamai merged 2 commits into
linode:developfrom
hkhalil-akamai:M3-9767-RegionTypeFilter-select
Apr 15, 2025
Merged

refactor: [M3-9767] - Use simple Select in RegionTypeFilter#12018
hkhalil-akamai merged 2 commits into
linode:developfrom
hkhalil-akamai:M3-9767-RegionTypeFilter-select

Conversation

@hkhalil-akamai

@hkhalil-akamai hkhalil-akamai commented Apr 11, 2025

Copy link
Copy Markdown
Contributor

Description 📝

Update RegionTypeFilter to use the new basic Select component instead of Autocomplete.

Changes 🔄

  • Update RegionTypeFilter to use basic Select
  • Drill regionFilter prop from LinodesLandingWrapper > LinodesLanding > DisplayLinodes
    • Previously the Autocomplete was used in uncontrolled mode and internally stored the current value of the select. The basic Select component does not support being uncontrolled.
    • Advantages: avoids multiple sources of truth for the currently active Region filter
    • Disadvantages: prop drilling
    • Alternatives: adding support for uncontrolled input in the simple Select component; custom React context

I invite feedback/suggestions for the best way to move forward.

Preview 📷

Before After
before.mov
after.mov

How to test 🧪

  • Navigate to the Linodes landing page
  • Select between "All", "Core" and "Distributed" region type filters
  • Verify only Linodes in the selected Region type are displayed
  • Refresh the page and verify the selection is maintained through local storage
Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 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


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@hkhalil-akamai
hkhalil-akamai marked this pull request as ready for review April 11, 2025 22:04
@hkhalil-akamai
hkhalil-akamai requested a review from a team as a code owner April 11, 2025 22:04
@hkhalil-akamai
hkhalil-akamai requested review from abailly-akamai and coliu-akamai and removed request for a team April 11, 2025 22:04
@linode-gh-bot

Copy link
Copy Markdown

Cloud Manager UI test results

🔺 2 failing tests on test run #2 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
2 Failing538 Passing4 Skipped104m 31s

Details

Failing Tests
SpecTest
smoke-community-stackscripts.spec.tsCloud Manager Cypress Tests→Community Stackscripts integration tests » Community Stackscripts integration tests
bucket-create-multicluster.spec.tsCloud Manager Cypress Tests→Object Storage Multicluster Bucket create » Object Storage Multicluster Bucket create

Troubleshooting

Use this command to re-run the failing tests:

pnpm cy:run -s "cypress/e2e/core/stackscripts/smoke-community-stackscripts.spec.ts,cypress/e2e/core/objectStorageMulticluster/bucket-create-multicluster.spec.ts"

@abailly-akamai abailly-akamai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am good with your approach here as I like keeping the component controlled.
I could be convinced otherwise but looks good to me!

Confirmed no regression with the control ✅

nit: address the linting issues

@harsh-akamai harsh-akamai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Verify only Linodes in the selected Region type are displayed
✅ Refresh the page and verify the selection is maintained through local storage

Changes look good! Thanks @hkhalil-akamai

@github-project-automation github-project-automation Bot moved this from Review to Approved in Cloud Manager Apr 15, 2025

@coliu-akamai coliu-akamai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thank you!

✅ confirmed testing instructions

@coliu-akamai coliu-akamai added the Approved Multiple approvals and ready to merge! label Apr 15, 2025
@hkhalil-akamai

Copy link
Copy Markdown
Contributor Author

Test failures appear unrelated.

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!

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants