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

fix: [M3-7533] - RegionSelect sorting in Firefox #9971

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Dec 6, 2023

Description 📝

This giant PR fixes a small issue with the region select DCs not being sorted properly in Firefox.

Changes 🔄

  • Adjust RegionSelect sorting util

Preview 📷

Before (Firefox) After (Firefox)
Screenshot 2023-12-06 at 13 38 23 Screenshot 2023-12-06 at 13 33 18

How to test 🧪

Reproduction steps

  • on develop & while using firefox, go to a create flow featuring the RegionSelect and notice the DCs are not alphabetically sorted.

Verification steps

  • on this branch & while using firefox, go to a create flow featuring the RegionSelect and notice the DCs are now properly alphabetically sorted.
  • Ensure there's no regression in the sorting for other Browsers (chrome & safari most importantly)

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 Dec 6, 2023
@abailly-akamai abailly-akamai marked this pull request as ready for review December 6, 2023 18:38
@abailly-akamai abailly-akamai requested a review from a team as a code owner December 6, 2023 18:38
@abailly-akamai abailly-akamai requested review from mjac0bs, cpathipa and coliu-akamai and removed request for a team December 6, 2023 18:38
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.

biggest pr i've ever seen 😆

Confirmed regions are now sorted on Firefox, and chrome + safari are the same ✅

@coliu-akamai coliu-akamai added the Add'tl Approval Needed Waiting on another approval! label Dec 6, 2023
Copy link

github-actions bot commented Dec 6, 2023

Coverage Report:
Base Coverage: 86.51%
Current Coverage: 86.49%

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

✅ Regions are alphabetically sorted in Firefox in the same order they are on Chrome and Safari.

❓ This might have been discussed in previous RegionSelect PRs and I'm lacking context, but did we recently decide to sort alphabetically by region regardless of country?

In prod, we sort alpha by country and so Toronto appears below Washington. In dev and this branch, Toronto is in alpha order within the US DCs. Same situation with Asia.

Prod Dev/This Branch
Screenshot 2023-12-06 at 1 37 05 PM Screenshot 2023-12-06 at 1 36 33 PM
Screenshot 2023-12-06 at 1 47 48 PM Screenshot 2023-12-06 at 1 47 17 PM

@abailly-akamai
Copy link
Contributor Author

@mjac0bs yup this has been discussed and approved by UI - they are in the process or refactoring-redesigning this component but in the meantime the alphabetical approach for ordering DCs within their groups has been pushed forward.

@abailly-akamai
Copy link
Contributor Author

@mjac0bs @coliu-akamai I also added grouping by country to make things a bit more readable on Mariah's suggestion - please feel free to take another look before I merge tomorrow. Thank you!

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Looks like that change resulted in a unit test failure because the test case is expecting a CA region to come before the US still, since they're both North America.
Screenshot 2023-12-06 at 3 07 59 PM

Edit: Also, this doesn't need a changeset, does it? This never made it to prod.

Approving since aside from that, I confirmed that sorting is happening by Region and then by Country in Chrome, Firefox, and Safari, which now matches prod. Thanks for making that country sort change - in the absence of final UX guidance, I think it is for the best that we keep prod's current sorting and easy visual navigation by flag. While it's not currently ideal for regions like Europe, I suspect that users in those regions are accustomed to their region's placement.

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Dec 6, 2023
@coliu-akamai
Copy link
Contributor

thanks Alban and Mariah! Reconfirmed sorting + confirmed grouping by country

@cpathipa
Copy link
Contributor

cpathipa commented Dec 7, 2023

Observation: DC : Singapore, SG is disabled in Chrome. But, enabled in Safari and Firefox (Able to select this option).

(Chrome) (Firefox & safari)
image image

Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Only observation found is DC: Singapore, SG was enabled in Firefox and safari. But, disabled in chrome.

@abailly-akamai
Copy link
Contributor Author

@cpathipa I can't reproduce. Please make sure you are testing:

  • on the same account
  • with the DC Get Well flag enabled
  • with MSW on

@cpathipa
Copy link
Contributor

cpathipa commented Dec 7, 2023

@cpathipa I can't reproduce. Please make sure you are testing:

  • on the same account
  • with the DC Get Well flag enabled
  • with MSW on

@abailly-akamai Sorry, My bad, that's due to different account in chrome. Confirming that sorting is working as expected in Firefox and no regression found.

@abailly-akamai abailly-akamai merged commit d6a9890 into linode:develop Dec 7, 2023
13 checks passed
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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants