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.

Sign up for GitHub

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

Jump to bottom

upcoming: [M3-8340] - Update Manage Image Regions Drawer based on UX feedback #10674

Merged
merged 6 commits into from
Jul 15, 2024
Merged

upcoming: [M3-8340] - Update Manage Image Regions Drawer based on UX feedback #10674

merged 6 commits into from
Jul 15, 2024

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Jul 11, 2024 •
edited
Loading

Description 📝

Updates the "Manage Image Regions" Drawer based on UX feedback 🖌️

Changes 🔄

  • The Manage Regions Multi-Select should not show any tags in the TextField 🚫🏷️
  • Regions should not be filtered from the Multi-Select 🌐
  • Make last region immutable when there is only one region left in the list ✖️
  • Close the drawer when "Save" is pressed (and the API request is successful 📕

Preview 📷

Before After
Screen.Recording.2024-07-11.at.12.41.49.PM.mov
Screen.Recording.2024-07-11.at.12.40.19.PM.mov

How to test 🧪

Prerequisites

  • Turn on Image Service Gen 2 feature flag
  • Turn on the MSW

Verification steps

  • Test the "Manage Regions" feature on the Image called multi-regions-test-image 🧪
    • Verify all of the changes listed above are implemented

As an Author I have 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

@bnussman-akamai bnussman-akamai added UX/UI Changes for UI/UX to review Image Service Gen2 labels Jul 11, 2024
@bnussman-akamai bnussman-akamai self-assigned this Jul 11, 2024
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner July 11, 2024 16:45
@bnussman-akamai bnussman-akamai requested review from hkhalil-akamai and abailly-akamai and removed request for a team July 11, 2024 16:45
@bnussman-akamai bnussman-akamai changed the title update to use new ux upcoming: [M3-8340] - Update Manage Image Regions Drawer based on UX feedback Jul 11, 2024
bnussman-akamai
bnussman-akamai commented Jul 11, 2024
View reviewed changes
packages/manager/src/components/RegionSelect/RegionMultiSelect.tsx
@@ -67,7 +67,7 @@ export const RegionMultiSelect = React.memo((props: RegionMultiSelectProps) => {
selectedIds,
sortRegionOptions,
width,
onClose,
...rest
Copy link
Member Author

Choose a reason for hiding this comment

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

Allows us to override any prop of the Autocomplete.

Needed to make this change so I could override renderTags

packages/manager/src/components/RegionSelect/RegionSelect.types.ts
@@ -68,7 +68,7 @@ export interface RegionSelectProps<

export interface RegionMultiSelectProps
extends Omit<
EnhancedAutocompleteProps<Region, false>,
EnhancedAutocompleteProps<Region, true>,
Copy link
Member Author

Choose a reason for hiding this comment

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

The isMulti generic should be true for the RegionMultiSelectProps. This type was wrong.

packages/manager/src/components/RegionSelect/RegionMultiSelect.tsx
@@ -155,7 +155,6 @@ export const RegionMultiSelect = React.memo((props: RegionMultiSelectProps) => {
InputProps: {
required,
},
placeholder: selectedRegions.length > 0 ? '' : placeholder,
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is a good default so I moved this special logic into AccessKeyRegions.tsx.

We want the placeholder to always show in the Manage Regions Drawer and this logic was messing that up

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 11, 2024 •
edited
Loading

Coverage Report: ✅
Base Coverage: 82.48%
Current Coverage: 82.48%

@bnussman-akamai bnussman-akamai requested a review from a team as a code owner July 11, 2024 17:34
@bnussman-akamai bnussman-akamai requested review from AzureLatte and removed request for a team July 11, 2024 17:34
hkhalil-akamai
hkhalil-akamai approved these changes Jul 11, 2024
View reviewed changes
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.

I'm strongly liking this new iteration of the manage regions flow and I think it improves on a lot of the usability quirks of the original version.

Verified all the changes and the existing functionality continue to work as expected. Also verified RegionMultiSelect works as expected in AccessKeyRegions.

Thanks for the cleanup in RegionMultiSelect and for test coverage.

...es/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyRegions/AccessKeyRegions.tsx Outdated
@@ -29,13 +29,15 @@ export const AccessKeyRegions = (props: Props) => {

return (
<RegionMultiSelect
onChange={onChange}
placeholder={
selectedRegion.length > 0 ? ' ' : 'Select regions or type to search'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for this space?

Suggested change
selectedRegion.length > 0 ? ' ' : 'Select regions or type to search'
selectedRegion.length > 0 ? '' : 'Select regions or type to search'

Copy link
Member Author

Choose a reason for hiding this comment

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

If I leave it as '' (with no space) the default placeholder shows, which isn't what we want

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, fixed with 6df361f

jaalah-akamai
jaalah-akamai approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

Nice! Really happy about this change 🥇

@jaalah-akamai jaalah-akamai added the Approved Multiple approvals and ready to merge! label Jul 15, 2024
@bnussman-akamai bnussman-akamai merged commit c71cdbc into linode:develop Jul 15, 2024
19 checks passed
This was referenced Jul 17, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hkhalil-akamai hkhalil-akamai hkhalil-akamai approved these changes

@jaalah-akamai jaalah-akamai jaalah-akamai approved these changes

@abailly-akamai abailly-akamai abailly-akamai approved these changes

@AzureLatte AzureLatte Awaiting requested review from AzureLatte AzureLatte is a code owner automatically assigned from linode/frontend-sdet

Assignees

@bnussman-akamai bnussman-akamai

Labels
Approved Multiple approvals and ready to merge! Image Service Gen2 UX/UI Changes for UI/UX to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bnussman-akamai @hkhalil-akamai @jaalah-akamai @abailly-akamai @bnussman