Skip to content

Conversation

@charliepark
Copy link
Contributor

Fixes #1965

This PR adds a table of External IPs to the instance's networking tab, and gives the user the option to attach (new) or detach (existing) external IPs.
Screenshot 2024-04-05 at 9 16 31 PM

I believe the PR is basically ready to go, with two caveats.

The smaller one is that while the e2e test works, part of it is a little awkward right now. I tried variations of
await dialog.getByRole('option', { name: 'rootbeer-float' }).click(), replacing option with button, etc., but couldn't seem to get the locator to match correctly. I have a keyboard-based hack that's working, but I'd rather have the proper Playwright locator working.

Another issue is that I'm not sure what we want the actions column for the Ephemeral IP rows to include. Detaching ephemeral IPs? Something else? I've included a placeholder "Copy IP address" for now so there's something in the dropdown, but I suspect we'll want to do something else.

@vercel
Copy link

vercel bot commented Apr 6, 2024

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

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Apr 12, 2024 5:38pm

@charliepark charliepark requested a review from david-crespo April 6, 2024 04:19
@benjaminleonard
Copy link
Contributor

Detach and copy IP address sounds good to me for ephemeral IPs.

@david-crespo
Copy link
Collaborator

@benjaminleonard @paryhin I think the table titles don't stand out enough as text-mono-sm and look too similar to the tab button text. Unfortunately sans-semi-lg stands out a bit too much, but it also doesn't look good in text-secondary or md size.

image

@paryhin
Copy link

paryhin commented Apr 9, 2024

@david-crespo This is what I have in design, I've been using sans-reg-lg:

https://www.figma.com/file/zIBsgP7gAkhapyNvVOqsn6/Floating-IPs?type=design&node-id=125%3A20767&mode=design&t=k0m3pa7XAojobIQJ-1

One issue is that we are using text-mono-sm everywhere else, so if we introduce a new table title style we'll need to do it in other places too.

@david-crespo
Copy link
Collaborator

I don't mind changing them all! There aren't that many that have their own titles.

@david-crespo
Copy link
Collaborator

I think it has to be text-secondary still, unfortunately.

image

Otherwise it's the only bright white on the page and it sticks out.

image

@david-crespo
Copy link
Collaborator

I'm starting to think that text-secondary could be a little brighter if it's actually our main text color.

@paryhin
Copy link

paryhin commented Apr 11, 2024

The secondary text color has to differ from the primary, to play well together and have enough contrast between them. Table headlines that I currently see in the preview (sans-lg, secondary) look good to me. I would only tweak spacing to bring them closer to the tables, as currently they are slightly detached.

@david-crespo
Copy link
Collaborator

I think it might be nice to say the word "floating" on floating IPs. I don't see why only Ephemeral deserves a badge. I see the pool column in the design — I wish we could do that, but the API doesn't tell us the pool right now. I'll have to look and see how hard that would be.

@david-crespo
Copy link
Collaborator

@benjaminleonard and I decided to move kind into its own column and label both kinds.

image

@david-crespo david-crespo merged commit a433d71 into main Apr 12, 2024
@david-crespo david-crespo deleted the networking-tab-enhancements branch April 12, 2024 17:48
david-crespo added a commit that referenced this pull request Apr 12, 2024
* clean up profile page visually. it doesn't need to be a form

* don't use toast. just add toast, remove toast. be toast
@paryhin
Copy link

paryhin commented Apr 12, 2024

@david-crespo Not sure why you would do that as only 1 IP can be ephemeral, and all others are floating IPs

@paryhin
Copy link

paryhin commented Apr 12, 2024

Also, only floating IPs can have names, it feels redundant to have a separate "kind" column in this case.

@david-crespo
Copy link
Collaborator

I think we have to say the word "floating" somewhere. It doesn't have to be in a separate column. Since ephemeral IPs will never have a name or description, I almost want to move them into their own table. Then we could solve this in the table titles.

david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Apr 14, 2024
oxidecomputer/console@7e34c11...2ba444c

* [2ba444ca](oxidecomputer/console@2ba444ca) smarter warning suppression for unhandled routes in msw
* [0de42104](oxidecomputer/console@0de42104) oxidecomputer/console#2146
* [5cae2111](oxidecomputer/console@5cae2111) turn off links test for now. it freaks me out
* [37470900](oxidecomputer/console@37470900) bump most deps
* [ff67b406](oxidecomputer/console@ff67b406) oxidecomputer/console#2145
* [f1e8f2ee](oxidecomputer/console@f1e8f2ee) oxidecomputer/console#2139
* [31508df8](oxidecomputer/console@31508df8) bring instance networking floating ip detach modal copy in line with floating IPs page
* [a433d71a](oxidecomputer/console@a433d71a) oxidecomputer/console#2130
* [83ace42b](oxidecomputer/console@83ace42b) oxidecomputer/console#2141
* [9270a930](oxidecomputer/console@9270a930) oxidecomputer/console#2134
* [00a03637](oxidecomputer/console@00a03637) update api-diff for better way of calling oxide.ts
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Apr 14, 2024
oxidecomputer/console@7e34c11...2ba444c

* [2ba444ca](oxidecomputer/console@2ba444ca)
smarter warning suppression for unhandled routes in msw
* [0de42104](oxidecomputer/console@0de42104)
oxidecomputer/console#2146
* [5cae2111](oxidecomputer/console@5cae2111)
turn off links test for now. it freaks me out
* [37470900](oxidecomputer/console@37470900)
bump most deps
* [ff67b406](oxidecomputer/console@ff67b406)
oxidecomputer/console#2145
* [f1e8f2ee](oxidecomputer/console@f1e8f2ee)
oxidecomputer/console#2139
* [31508df8](oxidecomputer/console@31508df8)
bring instance networking floating ip detach modal copy in line with
floating IPs page
* [a433d71a](oxidecomputer/console@a433d71a)
oxidecomputer/console#2130
* [83ace42b](oxidecomputer/console@83ace42b)
oxidecomputer/console#2141
* [9270a930](oxidecomputer/console@9270a930)
oxidecomputer/console#2134
* [00a03637](oxidecomputer/console@00a03637)
update api-diff for better way of calling oxide.ts
@paryhin
Copy link

paryhin commented Apr 15, 2024

As we can have only one Ephemeral IP, a separate table would be redundant. If we want to mention floating IPs, we can rename the Name column into Floating IP Name:

CleanShot 2024-04-15 at 10 43 08@2x

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.

List floating IPs more clearly on single instance page

5 participants