Skip to content

Add state and policy columns to physical disks table #2151

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

Merged
merged 6 commits into from
Apr 16, 2024
Merged

Conversation

charliepark
Copy link
Contributor

Fixes #2124

This PR adds two columns to the table listing physical disks:
Screenshot 2024-04-15 at 8 56 23 PM

Copy link

vercel bot commented Apr 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Apr 16, 2024 9:19pm

@david-crespo
Copy link
Collaborator

Nice. Most likely we don’t want those red because it’s not really an error state. It’s something that happens on purpose. Could do neutral or whatever the gray one is.

@charliepark
Copy link
Contributor Author

With the neutral badges:
Screenshot 2024-04-16 at 10 12 20 AM

@david-crespo
Copy link
Collaborator

Yeah that looks better. I think the policy column should be first because from Sean's comment, it sounds like policy drives the state, in the sense that you set the policy to expunged and wait for the state to change to decommissioned.

Another idea, but I doubt it can be made to feel right, is that we signal that the expunged/active state is temporary with a yellow badge color, but it's kind of weird because the text of the state is the same — active — so it would be stupid to signal it with a different color. I would say don't bother getting clever with this. We can mull it over and see if we come up with a cute approach. I think the issue is that while there are two fields here, there are really three states (as Sean's comment indicates) and the states are a composite of the two values. So I picture something like this:

  • In service + active: green
  • Expunged + active: yellow
  • Expunged + decommissioned: gray

But is that one column? What do you even call the column? Policy/state? Do you have two badges? One badge with a slash in it? Blech. Just some thoughts. I don't think any of this works, maybe @paryhin can make sense of it.

@charliepark
Copy link
Contributor Author

Ah, great point on the column ordering (or at least, the attribute ordering, if we move to one column). Do we know what the time delta is on expunging → decommissioned? We aren't live-updating the page right now, but I'm wondering how often they'd end up in an expunged + active situation.

@charliepark
Copy link
Contributor Author

Proper order:
Screenshot 2024-04-16 at 11 39 18 AM

Copy link
Collaborator

@david-crespo david-crespo 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, add these columns to the test here and it's good to go:

test('Disk inventory page', async ({ page }) => {
await page.goto('/system/inventory/disks')
await expectVisible(page, ['role=heading[name*="Inventory"]'])
const disksTab = page.getByRole('tab', { name: 'Disks' })
await expect(disksTab).toBeVisible()
await expect(disksTab).toHaveClass(/is-selected/)
const table = page.getByRole('table')
await expectRowVisible(table, { id: physicalDisks[0].id, 'Form factor': 'U.2' })
await expectRowVisible(table, { id: physicalDisks[3].id, 'Form factor': 'M.2' })
})

@david-crespo david-crespo merged commit e2b0a7d into main Apr 16, 2024
@david-crespo david-crespo deleted the more_disk_info branch April 16, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show more data about physical disks
2 participants