Skip to content

Conversation

@charliepark
Copy link
Contributor

This PR does a few things:

  1. Per Show image size on image picker #1824, it adds the size of the image to the option in the image picker. It rounds the image size to one decimal place. (We already had a bytesToGiB function that defaulted to 2 decimal points; I updated that function to receive a digits prop (like the round function's prop) and set it, in this case, to 1 decimal point.) Note: Late in testing I noticed … it appears that the API is sending back an already-rounded-to-the-next-largest-GiB value, so we should look into whether that behavior is expected/wanted, though we can do that in a separate PR.
  2. Per Image name looks bad when OS and/or version are empty #1777, it cleans up the label when data is missing from the image metadata (e.g. os, version). There might be some changes to the image upload form to change how that metadata is handled, but this at least makes it less awkward if data is missing. As there'll be the image's size now, there'll probably always be some data, but we won't have the awkward comma delineation we currently have when some data is missing.
  3. Per Active image select item should use accent colour #1830, it fixes the color of the metadata in the selected listbox item.

When decimals are showing below, it's because I forced a larger size. But these screenshots show what would happen:
Screenshot 2023-12-05 at 7 27 38 PM
Screenshot 2023-12-05 at 8 59 54 PM


Up in item 1, I noted that the API is reporting image sizes as the next GiB up. Here you can see the API response calling the images, with this particular file reporting a size of 1073741824:
Screenshot 2023-12-05 at 9 16 32 PM

@vercel
Copy link

vercel bot commented Dec 6, 2023

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

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Dec 6, 2023 7:53pm

expect(round(1.9, 1)).toEqual(1.9)
expect(round(5 / 2, 2)).toEqual(2.5) // math expressions are resolved
expect(round(1879048192 / GiB, 2)).toEqual(1.75) // constants can be evaluated
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

we stan a testing king

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, I was mostly trying to figure out why my 1.2GiB image kept getting represented as 2GiB, and hadn't yet looked at the API response. This is probably ultimately just testing Math.round, but I figured we could keep it around.

)}
{i}
</span>
))

This comment was marked as resolved.

This comment was marked as resolved.

@david-crespo
Copy link
Collaborator

david-crespo commented Dec 6, 2023

I don't get what you mean about the API rounding up — you can show me tomorrow.

@david-crespo
Copy link
Collaborator

Ah, I think I know what's going on there. Images are created from snapshots, and snapshots are created from disks. Even in the case of images uploaded directly through the console, the way it works is: we create a disk in a special importing state, upload the file to it in chunks, then snapshot it and turn the snapshot into an image. But disk size is required to be an integer multiple of 1 GiB:

https://github.com/oxidecomputer/omicron/blob/001143cf3b686b8cb4a72916e9f775a0a186df9b/nexus/src/app/disk.rs#L164-L174

So in practice it would seem the rounding really isn't necessary. I'd leave it in anyway.

<div className="text-secondary">
{i.os} <Slash /> {i.version} {projectSiloIndicator}
<div>{name}</div>
<div className="text-tertiary selected:text-accent-secondary">
Copy link
Contributor

Choose a reason for hiding this comment

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

Small tweak to match component in Figma library


const Slash = () => <span className="mx-0.5 text-quinary">/</span>
const Slash = () => (
<span className="mx-0.5 text-quinary selected:text-accent-disabled">/</span>
Copy link
Contributor

@benjaminleonard benjaminleonard Dec 6, 2023

Choose a reason for hiding this comment

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

Added this because it's visually correct but using the disabled token here is bad vibes. We should extend the accent tokens to add a quaternary step which is equivalent to it so that it's a little more semantic.

If you're interested the accent and content tokens don't have quite so many steps in them because you can't squeeze out as much contrast from the colours as you can from neutrals.

Copy link
Contributor

Choose a reason for hiding this comment

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

@charliepark Let me know if you want to run through the process of updating the colour tokens with me.

@charliepark
Copy link
Contributor Author

Just wanted to add a screenshot to log the color updates from Ben's commit:
Screenshot 2023-12-06 at 11 34 19 AM

@david-crespo
Copy link
Collaborator

Random thought, not sure if it needs a fix here or in the API — if OS and version can be non-empty whitespace-only strings (and I think they can) then this will still show the weird empty spot. We should probably reject such a string API-side, so I'm inclined not to give it special handling here.

@charliepark charliepark merged commit a0bf47a into main Dec 6, 2023
@charliepark charliepark deleted the 1824-show-image-size-on-image-picker branch December 6, 2023 20:31
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Dec 7, 2023
oxidecomputer/console@ae8218d...1802c28

* [1802c285](oxidecomputer/console@1802c285) oxidecomputer/console#1839
* [ce09b547](oxidecomputer/console@ce09b547) bump postcss-pseudo-classes for fake vuln
* [e09b803b](oxidecomputer/console@e09b803b) might as well get vitest 1.0 in there too
* [83dd73ee](oxidecomputer/console@83dd73ee) minor bumps for react router, msw, vite, tailwind, recharts
* [6ae6beeb](oxidecomputer/console@6ae6beeb) oxidecomputer/console#1829
* [a0bf47aa](oxidecomputer/console@a0bf47aa) oxidecomputer/console#1836
* [6c9420ad](oxidecomputer/console@6c9420ad) oxidecomputer/console#1835
* [64e97b01](oxidecomputer/console@64e97b01) api-diff also takes a commit
* [22bef0bb](oxidecomputer/console@22bef0bb) oxidecomputer/console#1833
* [2fe50f51](oxidecomputer/console@2fe50f51) oxidecomputer/console#1810
* [faadb6d3](oxidecomputer/console@faadb6d3) oxidecomputer/console#1832
* [9e82f9ab](oxidecomputer/console@9e82f9ab) oxidecomputer/console#1811
* [5e11fd83](oxidecomputer/console@5e11fd83) tweak api-diff
* [dae20577](oxidecomputer/console@dae20577) oxidecomputer/console#1827
* [ed0ef62e](oxidecomputer/console@ed0ef62e) minor tweaks to api-diff script
* [1c790d27](oxidecomputer/console@1c790d27) oxidecomputer/console#1819
* [97be7724](oxidecomputer/console@97be7724) oxidecomputer/console#1826
* [87f4d8b8](oxidecomputer/console@87f4d8b8) oxidecomputer/console#1814
* [65ae1212](oxidecomputer/console@65ae1212) oxidecomputer/console#1820
* [5a6dcea7](oxidecomputer/console@5a6dcea7) oxidecomputer/console#1822
* [4e1bbe13](oxidecomputer/console@4e1bbe13) oxidecomputer/console#1821
* [17408f64](oxidecomputer/console@17408f64) oxidecomputer/console#1813
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Dec 7, 2023
### User-facing changes

* [1802c285](oxidecomputer/console@1802c285)
oxidecomputer/console#1839
* [6ae6beeb](oxidecomputer/console@6ae6beeb)
oxidecomputer/console#1829
* [a0bf47aa](oxidecomputer/console@a0bf47aa)
oxidecomputer/console#1836
* [9e82f9ab](oxidecomputer/console@9e82f9ab)
oxidecomputer/console#1811
* [5a6dcea7](oxidecomputer/console@5a6dcea7)
oxidecomputer/console#1822

### All changes

oxidecomputer/console@ae8218d...1802c28

* [1802c285](oxidecomputer/console@1802c285)
oxidecomputer/console#1839
* [ce09b547](oxidecomputer/console@ce09b547)
bump postcss-pseudo-classes for fake vuln
* [e09b803b](oxidecomputer/console@e09b803b)
might as well get vitest 1.0 in there too
* [83dd73ee](oxidecomputer/console@83dd73ee)
minor bumps for react router, msw, vite, tailwind, recharts
* [6ae6beeb](oxidecomputer/console@6ae6beeb)
oxidecomputer/console#1829
* [a0bf47aa](oxidecomputer/console@a0bf47aa)
oxidecomputer/console#1836
* [6c9420ad](oxidecomputer/console@6c9420ad)
oxidecomputer/console#1835
* [64e97b01](oxidecomputer/console@64e97b01)
api-diff also takes a commit
* [22bef0bb](oxidecomputer/console@22bef0bb)
oxidecomputer/console#1833
* [2fe50f51](oxidecomputer/console@2fe50f51)
oxidecomputer/console#1810
* [faadb6d3](oxidecomputer/console@faadb6d3)
oxidecomputer/console#1832
* [9e82f9ab](oxidecomputer/console@9e82f9ab)
oxidecomputer/console#1811
* [5e11fd83](oxidecomputer/console@5e11fd83)
tweak api-diff
* [dae20577](oxidecomputer/console@dae20577)
oxidecomputer/console#1827
* [ed0ef62e](oxidecomputer/console@ed0ef62e)
minor tweaks to api-diff script
* [1c790d27](oxidecomputer/console@1c790d27)
oxidecomputer/console#1819
* [97be7724](oxidecomputer/console@97be7724)
oxidecomputer/console#1826
* [87f4d8b8](oxidecomputer/console@87f4d8b8)
oxidecomputer/console#1814
* [65ae1212](oxidecomputer/console@65ae1212)
oxidecomputer/console#1820
* [5a6dcea7](oxidecomputer/console@5a6dcea7)
oxidecomputer/console#1822
* [4e1bbe13](oxidecomputer/console@4e1bbe13)
oxidecomputer/console#1821
* [17408f64](oxidecomputer/console@17408f64)
oxidecomputer/console#1813
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.

Active image select item should use accent colour Show image size on image picker Image name looks bad when OS and/or version are empty

4 participants