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: update subnet table layout #5226

Merged

Conversation

Jay-Topher
Copy link
Contributor

Done

  • Update fabric and space tables to support grouping

QA steps

  • visit subnets page
  • Ensure that both fabric and space tables have row grouping

Fixes

Fixes: https://warthogs.atlassian.net/browse/MAASENG-2435

Screenshots

Fabric

image

Space

image

Notes

@webteam-app
Copy link

Demo starting at https://maas-ui-5226.demos.haus

Copy link
Contributor

@petermakowski petermakowski left a comment

Choose a reason for hiding this comment

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

A few things to improve below.

The items count in each group row should be the total number of rows in this group across all pages, not just the current page.

Rows nested in a group should have increased indentation, like in the machine list.

I'm pretty sure that each group name in the row group should be a link (a link to fabric or space depending on the grouping). Could you talk to @amylily1011 or @dgtlntv and verify this, plus any potential design considerations.

@Jay-Topher Jay-Topher force-pushed the MAASENG-2435-update-subnet-table-layout branch from f71da18 to 2eb9674 Compare December 5, 2023 11:59
@Jay-Topher Jay-Topher marked this pull request as ready for review December 5, 2023 12:00
@Jay-Topher Jay-Topher requested a review from ndv99 December 5, 2023 12:01
@Jay-Topher Jay-Topher force-pushed the MAASENG-2435-update-subnet-table-layout branch from 2eb9674 to 5734e36 Compare December 5, 2023 12:42
@Jay-Topher Jay-Topher requested a review from ndv99 December 5, 2023 12:43
Copy link
Contributor

@ndv99 ndv99 left a comment

Choose a reason for hiding this comment

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

This looks good IMO, nice one!

@ndv99
Copy link
Contributor

ndv99 commented Dec 5, 2023

A few things to improve below.

The items count in each group row should be the total number of rows in this group across all pages, not just the current page.

Rows nested in a group should have increased indentation, like in the machine list.

I'm pretty sure that each group name in the row group should be a link (a link to fabric or space depending on the grouping). Could you talk to @amylily1011 or @dgtlntv and verify this, plus any potential design considerations.

Before merging - @Jay-Topher did you follow up on this?

@Jay-Topher
Copy link
Contributor Author

A few things to improve below.
The items count in each group row should be the total number of rows in this group across all pages, not just the current page.
Rows nested in a group should have increased indentation, like in the machine list.
I'm pretty sure that each group name in the row group should be a link (a link to fabric or space depending on the grouping). Could you talk to @amylily1011 or @dgtlntv and verify this, plus any potential design considerations.

Before merging - @Jay-Topher did you follow up on this?

Yes, I did, you could verify also but I'm sure I covered everything there

@petermakowski
Copy link
Contributor

@Jay-Topher There should be a different line length around indented rows to help separate them out visually. Would be great to align this with machine listing.

machine listing

Google Chrome screenshot 001186@2x

subnets

Google Chrome screenshot 001184@2x

@Jay-Topher Jay-Topher force-pushed the MAASENG-2435-update-subnet-table-layout branch from 5734e36 to dd8cb95 Compare December 6, 2023 09:46
@Jay-Topher Jay-Topher merged commit f365399 into canonical:main Dec 6, 2023
5 checks passed
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.

4 participants