Skip to content

upcoming: [M3-8051] - Set PlacementGroupSelect clearOnBlur#10427

Merged
abailly-akamai merged 2 commits intolinode:developfrom
abailly-akamai:M3-8051
May 1, 2024
Merged

upcoming: [M3-8051] - Set PlacementGroupSelect clearOnBlur#10427
abailly-akamai merged 2 commits intolinode:developfrom
abailly-akamai:M3-8051

Conversation

@abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented May 1, 2024

Description 📝

Small fix to make sure the PlacementGroupSelect gets its selected option cleared when no value is selected. In the Linode create flow, we clear the value when the selectedRegionId is updated. While the value is cleared and everything works as expected, an artifact of not clearing the input results in the selected option still showing in the select instead of the desired "None" placeholder.

from mui.com

clearOnBlur

If true, the input's text is cleared on blur if no value is selected. Set it to true if you want to help the user enter a new value. Set it to false if you want to help the user resume their search.

Changes 🔄

  • Set PlacementGroupSelect clearOnBlur to true

Target release date 🗓️

5/13/2024

Preview 📷

Before After
Screen.Recording.2024-04-30.at.22.42.39.mov
Screen.Recording.2024-04-30.at.22.40.38.mov

How to test 🧪

Verification steps

See videos above. Using ALPHA

  • Navigate to http://localhost:3000/linodes/create
  • Select Newark, NJ for the region
  • Select a PG
  • Select a different region
  • Confirm the option is reset to the placeholder as opposed to the previously selected option

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

@abailly-akamai abailly-akamai self-assigned this May 1, 2024
@abailly-akamai abailly-akamai marked this pull request as ready for review May 1, 2024 02:55
@abailly-akamai abailly-akamai requested a review from a team as a code owner May 1, 2024 02:55
@abailly-akamai abailly-akamai requested review from bnussman-akamai and mjac0bs and removed request for a team May 1, 2024 02:55
@github-actions
Copy link

github-actions bot commented May 1, 2024

Coverage Report:
Base Coverage: 81.82%
Current Coverage: 81.83%

@carrillo-erik
Copy link
Contributor

Nice and easy fix. Verified that the component value clears as expected.

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Confirmed the change worked as described ✅

I wonder why we decided to default clearOnBlur to false in Autocomplete. I'm feeling like we should remove that override and use MUI's default (which would be true)


This stuff doesn't need to be addressed in this PR, but:

I noticed that PlacementGroupSelect.tsx uses useAllPlacementGroupsQuery and client side filters by region. We could API filter here if we wanted.

I also notice that PlacementGroupsDetailPanel maintains its own state. I was getting very confused when testing this because PlacementGroupsDetailPanel's selectedPlacementGroup gets out of sync with LinodeCreate's placementGroupSelection.

For example, if you change regions, placementGroupSelection gets set to undefined, but in PlacementGroupsDetailPanel it still has a selected placement group in selectedPlacementGroup.

The only reason the PlacementGroupsSelect updates to show "None" is because the region filter changed, not because selectedPlacementGroup has been set to null or undefined.

Everything works, it just feels very round-about in relation to the state

@abailly-akamai
Copy link
Contributor Author

@bnussman-akamai i also noticed that while implementing this fix but wanted to keep the scope small as we are closing on the feature for production and as you mentioned it is "working". However your astute comments are not going unnoticed and made their way to M3-7993 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants