Skip to content

upcoming: [M3-7354] DC Get Well - Disabled regions #9909

Merged
abailly-akamai merged 10 commits intolinode:developfrom
abailly-akamai:M3-7354
Nov 28, 2023
Merged

upcoming: [M3-7354] DC Get Well - Disabled regions #9909
abailly-akamai merged 10 commits intolinode:developfrom
abailly-akamai:M3-7354

Conversation

@abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Nov 15, 2023

Description 📝

This PR implements the UI for disabled regions in the <RegionSelect />. We originally had hardcoded "fake" regions to represent disabled menu items. DC Get Well now implements a new endpoint (account/availability) which represents an array of DCs (or regions) unavailable to the user.

The UI is essentially the same (disabled item plus tooltip with link to a static info page - link TBD).

Right now this PR does not implement the behavior in all RegionSelect instances to keep the PR small and focus on pure logic implementation. (see testing steps)

Changes 🔄

  • Implement new account/availability request (mocked for now) and underlying logic to determine the region's availability to a given user.
  • Implement a new prop to pass the current capability to the region select
  • remove all logic pertaining to fake regions 🎉
  • cleanup and enhanced test suites

Preview 📷

Screenshot 2023-11-21 at 13 35 40

How to test 🧪

Prerequisites

  • Pull code locally
  • Turn on the DC Get Well feature flag in dev settings
  • Turn on MSW

Verification steps

  • You can verify the behavior by adding the current capability prop (you can try for various create flows)

ex: add currentCapability="Block Storage" to the VolumeCreate region select

👉 verify that Atlanta and Tokyo are disabled items

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 Nov 15, 2023
@abailly-akamai abailly-akamai added DC Get Well Relating to the DC Get Well project Work in Progress labels Nov 15, 2023
@abailly-akamai abailly-akamai force-pushed the M3-7354 branch 5 times, most recently from ffe8667 to 743c345 Compare November 21, 2023 18:20
@abailly-akamai abailly-akamai changed the title M3-7354 change: [M3-7354] DC Get Well - Disabled regions Nov 21, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding loading props since we now have an API request that may affect the rendering. It's unlikely to be noticable at all and i doubt we'll ever see the loading state but does not hurt to plan for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note this the comment above. The prop IS going to be required when we wrap up this implementation. But for the sake of keeping the PR small and not add the prop on every select (there's some slight extra work to do for some of our services) I kept it optional. right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

buh-bye! 🎉

@abailly-akamai abailly-akamai marked this pull request as ready for review November 21, 2023 18:53
@abailly-akamai abailly-akamai requested a review from a team as a code owner November 21, 2023 18:53
@abailly-akamai abailly-akamai requested review from coliu-akamai and hana-akamai and removed request for a team November 21, 2023 18:53
@abailly-akamai abailly-akamai changed the title change: [M3-7354] DC Get Well - Disabled regions upcoming: [M3-7354] DC Get Well - Disabled regions Nov 21, 2023
Copy link
Contributor

@coliu-akamai coliu-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 current capability prop works as expected on flows
✅ general code review

Good test additions + I liked how you passed in objects to the region utility functions, definitely helped with readability!

@coliu-akamai coliu-akamai added the Add'tl Approval Needed Waiting on another approval! label Nov 21, 2023
@abailly-akamai
Copy link
Contributor Author

VPC test failure looks unrelated. It passed locally every time I ran it locally

Copy link
Contributor

@hana-akamai hana-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 only seeing Tokyo, JP as a disabled item (with MSW on) 🤔

It seems like there was some kind of regression with keyboard accessibility. The scroll bar no longer follows the user. The padding between each region has increased as well (not sure if that was intentional)

This branch Remote dev
this.branch.mov
Screen.Recording.2023-11-27.at.2.58.37.PM.mov

@abailly-akamai
Copy link
Contributor Author

@hana-linode Great catch! this issue was not stemming from this PR (was a result of the RegionSelect refactor) but an issue nonetheless and one that won't make it to production 🙇 (I am assuming when you labelled "Remote dev" it was actually prod cause the RegionSelect refactor has not made it to prod yet)

I fixed both the mobile padding and the scrolling issue. As far as Miami is concerned, that may only have to do the fact you don't have Miami in the list all together?

@abailly-akamai
Copy link
Contributor Author

abailly-akamai commented Nov 27, 2023

@coliu-akamai can you please give the PR another run after this fix? wanna make sure we don't have any other regressions as a result - thx!

@coliu-akamai
Copy link
Contributor

thank you @hana-linode for catching this!!! 🙏

taking another look through it now

@coliu-akamai
Copy link
Contributor

  • Confirmed keyboard accessibility works now! (had one question - for disabled items' tooltip, is there anyway to make that available for keyboard and voiceover users?)
  • tests pass
  • region select still works as expected with capabilities

@hana-linode looked into this a bit - Miami wasn't in the cached regions.json data that the MSW uses, so it also didn't show up for me here. I added 'Block Storage' to some other regions that are in regions.json and they're disabled 😄
image

does anyone know how the regions.json file gets generated? If Miami isn't consistently included in that file, could we switch Miami out for a region that is?

@abailly-akamai
Copy link
Contributor Author

@coliu-akamai thanks for the second pass! Glad it's working now.

  1. Good point. I'll make a follow up item to handle accessibility for disabled items
  2. I'll make a second item to get rid of miami, use a better region for MSW and update tests

I don't wanna block the rest of the work we have to implement and those can def be follow up items

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Volume Create flow LGTM.

I'm not seeing the disabled regions for Linode Create though. I'm adding currentCapability="Block Storage" to the RegionSelect in SelectRegionPanel. Is there something I'm missing?

@coliu-akamai
Copy link
Contributor

coliu-akamai commented Nov 28, 2023

hmm I'm actually seeing the same thing 😢 looking into it

edit: maybe the MSW is being strange?? when I reload the page nothing's disabled, but then when I cmd-s in a file (save my file changes), it started working (stubbed in Newark): 😖
image

@abailly-akamai
Copy link
Contributor Author

abailly-akamai commented Nov 28, 2023

@hana-linode & @coliu-akamai

Fixed a couple issues:

  1. there was an issue with the memoization of regions which resulted in not seeing the proper disabled region when navigating the APP.
  2. i replaced miami with atlanta so our results are consistent
  3. i fixed the css warning

Now, for

  • SelectRegionPanel: currentCapability="Linodes" => Singapore and Tokyo should be disabled
  • VolumeCreate: currentCapability="Block Storage" => Atlanta and Tokyo should be disabled
    Which corresponds to our MSW mocked data 👍

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

  • reconfirmed keyboard accessibility
  • reconfirmed tests
  • confirmed RegionSelect works as expected when going to different flows for both Linode Create, Volumes Create, and other flows (tested Kubernetes, ObjectStorage)
  • confirmed styling warning is gone
  • ++ nice addition with sticky continent headers for the region select 🚀

ty to Hana for the sharp eyes and Alban for the fixes!

Copy link
Contributor

@hana-akamai hana-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 keyboard accessibility
  • RegionSelect disabled state for Volume Create and Linode Create
  • No more console warning for styling

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Nov 28, 2023
@abailly-akamai abailly-akamai merged commit 4c6ddd6 into linode:develop Nov 28, 2023
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! DC Get Well Relating to the DC Get Well project Work in Progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants