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

feat: [M3-7948] - Add Region filtering to Linodes landing table #10639

Conversation

hana-akamai
Copy link
Contributor

@hana-akamai hana-akamai commented Jul 2, 2024

Description 📝

Add support for region filtering in the Linodes landing table for Gecko LA. Region filter is saved in local storage similar to the "Show X" dropdown.

Changes 🔄

List any change relevant to the reviewer.

  • Add site_type to the linode instance type
  • Removed edge references where possible
  • Add region filter dropdown to the Linodes landing table

Preview 📷

(ip address has been redacted)

Screen.Recording.2024-10-03.at.1.51.32.PM.mov

How to test 🧪

Verification steps

  • All should display all Linodes (core and distributed regions)
  • Core should only display Linodes in core regions
  • Distributed should only display Linodes in distributed regions
  • Ensure sorting, summary, and tag view still work as normal
  • Navigate to another page and go back to the Landing page, the region selection should stay the same

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

@hana-akamai hana-akamai added the Gecko LA Relating to Gecko LA label Jul 2, 2024
@hana-akamai hana-akamai self-assigned this Jul 2, 2024
@hana-akamai hana-akamai marked this pull request as ready for review July 16, 2024 21:13
@hana-akamai hana-akamai requested a review from a team as a code owner July 16, 2024 21:13
@hana-akamai hana-akamai requested review from mjac0bs and carrillo-erik and removed request for a team July 16, 2024 21:13
Copy link

github-actions bot commented Jul 16, 2024

Coverage Report:
Base Coverage: 87.22%
Current Coverage: 87.23%

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.

x-filter in mock requests looks good and so does the persistence of the selection.

packages/api-v4/src/linodes/types.ts Outdated Show resolved Hide resolved
@mjac0bs mjac0bs self-requested a review July 18, 2024 16:40
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.

Thanks for the changes! All as a default is great, card view looks good, and putting the region type filter in its own component is clean.

Let's get the merge conflict resolved and make sure CI passes. And I think we do actually need to extend the width of the autocomplete / add padding after all. The checkmark is too close to the selection:

Screenshot 2024-07-18 at 1 39 18 PM

@jaalah-akamai jaalah-akamai self-requested a review October 2, 2024 19:10
@hana-akamai hana-akamai requested a review from a team as a code owner October 3, 2024 17:17
@hana-akamai hana-akamai requested review from jdamore-linode and removed request for a team October 3, 2024 17:17
Comment on lines +227 to +231
{isGeckoLAEnabled && (
<Paper sx={{ padding: 1 }}>
<RegionTypeFilter handleRegionFilter={handleRegionFilter} />
</Paper>
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the main change

@@ -241,7 +247,7 @@ class ListLinodes extends React.Component<CombinedProps, State> {
return <CircleProgress />;
}

if (this.props.linodesData.length === 0) {
if (totalNumLinodes === 0 && linodesData.length === 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't just rely on the length of linodesData because the 0 could be coming from filtered data and the user could have still have linodes

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.

UI: I was seeing a minor issue when one of the categories has no Linodes, Summary View is selected, and we are not grouping by tags. Can we add a consistent amount of spacing before the "No items to display"?

Code: Removal of edge looked good. Saw just a few nits.

Screen.Recording.2024-10-03.at.3.43.04.PM.mov

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.

Looks good!

I wonder if we should add some kind of small loading state for when the user is switching between different region filters

@mjac0bs mjac0bs self-requested a review October 4, 2024 18:14
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.

🚀

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Oct 4, 2024
@hana-akamai
Copy link
Contributor Author

hana-akamai commented Oct 7, 2024

@bnussman-akamai Good point, just realized we don't use the TableRowLoading component in LinodesLanding. I'll create a separate ticket to address that since it's going to require some code maneuvering

Edit: M3-8717

@hana-akamai hana-akamai merged commit 5155c6a into linode:develop Oct 7, 2024
20 checks passed
Copy link

cypress bot commented Oct 7, 2024

Cloud Manager E2E    Run #6632

Run Properties:  status check failed Failed #6632  •  git commit 5155c6a0af: feat: [M3-7948] - Add Region filtering to Linodes landing table (#10639)
Project Cloud Manager E2E
Run status status check failed Failed #6632
Run duration 27m 29s
Commit git commit 5155c6a0af: feat: [M3-7948] - Add Region filtering to Linodes landing table (#10639)
Committer Hana Xu
View all properties for this run ↗︎

Test results
Tests that failed  Failures 1
Tests that were flaky  Flaky 4
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 424

Tests for review

Failed  cypress/e2e/core/placementGroups/delete-placement-groups.spec.ts • 1 failed test

View Output Video

Test Artifacts
Placement Group deletion > can delete with Linodes assigned when unexpected error show up and retry Screenshots Video
Flakiness  linodes/linode-config.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Linode Config management > End-to-End > Clones a config Screenshots Video
Flakiness  linodes/create-linode.spec.ts • 1 flaky test

View Output Video

Test Artifacts
... > creates a Shared CPU Linode Screenshots Video
Flakiness  linodes/rebuild-linode.spec.ts • 1 flaky test

View Output Video

Test Artifacts
rebuild linode > cannot rebuild a provisioning linode Screenshots Video
Flakiness  volumes/delete-volume.spec.ts • 1 flaky test

View Output Video

Test Artifacts
volume delete flow > deletes a volume Screenshots Video

hasyed-akamai pushed a commit to hasyed-akamai/manager that referenced this pull request Oct 9, 2024
…de#10639)

## Description 📝
Add support for region filtering in the Linodes landing table for Gecko LA. Region filter is saved in local storage similar to the "Show X" dropdown.

## Changes  🔄
List any change relevant to the reviewer.
- Add `site_type` to the linode instance type
- Removed `edge` references where possible
- Add region filter dropdown to the Linodes landing table

## How to test 🧪
### Verification steps
- `All` should display all Linodes (core and distributed regions)
- `Core` should only display Linodes in `core` regions
- `Distributed` should only display Linodes in `distributed` regions
- Ensure sorting, summary, and tag view still work as normal
- Navigate to another page and go back to the Landing page, the region selection should stay the same
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! Gecko LA Relating to Gecko LA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants