Skip to content

Conversation

@charliepark
Copy link
Contributor

@charliepark charliepark commented May 7, 2024

Fixes #1097
Fixes #1098

Currently, when the user creates an instance, we just create an ephemeral IP and attach it to the instance. With this PR, we give them a checkbox (default = checked) to let them opt out/in of the ephemeral IP creation step, and we give them a dropdown that lets them pick which IP pool to draw the ephemeral IP from.

We will also add the option to select a floating IP (#1099), but that'll be separate from this PR.

I'm not sure about the layout / copy I have in this PR, so would welcome design feedback from @paryhin and @benjaminleonard. @david-crespo and I were able to simplify the designs in Figma a bit (we can consolidate some of the options there), but I think Eugene will have good suggestions on what I have here.

Screenshot 2024-05-06 at 5 58 37 PM

@vercel
Copy link

vercel bot commented May 7, 2024

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

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview May 8, 2024 6:36pm

@charliepark
Copy link
Contributor Author

Ben had some suggestions, which I incorporated in the recent PRs. Here's the latest …
Screenshot 2024-05-07 at 11 35 43 AM
Main changes from Ben …

  • added microcopy and link to docs
  • made select/option dropdown conditionally render, based on whether box is checked or not (parallel with radio group above, with selective rendering of "network interface" second-level options

I know Eugene has a few thoughts as well.

@paryhin
Copy link

paryhin commented May 7, 2024

CleanShot 2024-05-07 at 23 26 25@2x

  1. We should keep the title for the IP pool input, as currently it is not clear what you are selecting.
  2. We should show "default" pool badge in the input in its rest state.
  3. If we want to have this type of long description with links, we may consider the following behavior:
    CleanShot 2024-05-07 at 23 23 18

@charliepark
Copy link
Contributor Author

I'm not against having some kind of <details> component to reveal the text content, but it might need a different visual treatment, if all of our other info icons have a tooltip?

@charliepark
Copy link
Contributor Author

The latest: Screenshot 2024-05-07 at 7 18 11 PM

@david-crespo
Copy link
Collaborator

david-crespo commented May 8, 2024

Looking good. How about “Allocate” instead of “Automatically assign”? Automatic is redundant, everything the form does is automatic. And “assign” isn’t really a term we use, it would be attach, though I could live with assign. “Allocate and attach” would be most precise but is kind of long.

@david-crespo
Copy link
Collaborator

I agree with Eugene that the fact that the listbox is for picking a pool should be indicated somehow. If the name of the pool wasn’t “ip-pool-1” the problem would be more obvious.

@charliepark
Copy link
Contributor Author

Ah, right; I'd meant to get that in there.

@charliepark
Copy link
Contributor Author

I can see what Eugene was getting at with the microcopy being on the screen. With the various titles and labels, it was getting crowded. I've put it into a tooltip for now, though that loses the link to find out more.
Screenshot 2024-05-07 at 9 39 00 PM

@paryhin
Copy link

paryhin commented May 8, 2024

Yeah, switching from a tooltip is probably a separate PR, we can keep it as it is for now. If we want the link, we may put it in the contextual docs popover. Everything else looks good to me.

@david-crespo
Copy link
Collaborator

david-crespo commented May 8, 2024

I think it needs more space and more hierarchy to the text. Below shows a sort of maximalist approach: extra 1rem under heading, checkbox, and listbox, extra 2.5rem above heading, heading one size bigger, checkbox label text-secondary. The size bump is a problem because it doesn't match the other ones. It makes me wonder if all the input labels in forms should be one size up, or at least whether we should have some type pattern that makes it easier to imply grouping.

image

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 great. Happy to have design follow up on the spacing and hierarchy stuff.

@charliepark charliepark merged commit b400ae7 into main May 8, 2024
@charliepark charliepark deleted the external-ip-selection branch May 8, 2024 18:49
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request May 22, 2024
oxidecomputer/console@078d171...a228b75

* [a228b75b](oxidecomputer/console@a228b75b) bump omicron (only one tiny diff in validators)
* [fc91ec1e](oxidecomputer/console@fc91ec1e) oxidecomputer/console#2256
* [39b4491e](oxidecomputer/console@39b4491e) oxidecomputer/console#2230
* [e4e912ca](oxidecomputer/console@e4e912ca) oxidecomputer/console#2247
* [dcf09ec9](oxidecomputer/console@dcf09ec9) oxidecomputer/console#2217
* [c36b3d63](oxidecomputer/console@c36b3d63) oxidecomputer/console#2238
* [a8eb7745](oxidecomputer/console@a8eb7745) oxidecomputer/console#2251
* [9b20b7c9](oxidecomputer/console@9b20b7c9) oxidecomputer/console#2248
* [f20a5bcb](oxidecomputer/console@f20a5bcb) oxidecomputer/console#2245
* [b815dd8f](oxidecomputer/console@b815dd8f) oxidecomputer/console#2244
* [8c7b2946](oxidecomputer/console@8c7b2946) add node_modules to eslint ignore patterns
* [90e78dbb](oxidecomputer/console@90e78dbb) oxidecomputer/console#2237
* [b603d2dd](oxidecomputer/console@b603d2dd) oxidecomputer/console#2242
* [bfce37c7](oxidecomputer/console@bfce37c7) upgrade @oxide/openapi-gen-ts to 0.2.2
* [efceb17d](oxidecomputer/console@efceb17d) oxidecomputer/console#2236
* [1aa46459](oxidecomputer/console@1aa46459) oxidecomputer/console#2235
* [b400ae78](oxidecomputer/console@b400ae78) oxidecomputer/console#2225
* [7bb3bbf7](oxidecomputer/console@7bb3bbf7) oxidecomputer/console#2229
* [c56a9ec5](oxidecomputer/console@c56a9ec5) oxidecomputer/console#2228
* [cd9d1f99](oxidecomputer/console@cd9d1f99) oxidecomputer/console#2227
* [ee269bd9](oxidecomputer/console@ee269bd9) oxidecomputer/console#2223
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request May 22, 2024
Highlights: soft image validation, logout button on error pages to help
deal with auth-related errors.

oxidecomputer/console@078d171...a228b75

* [a228b75b](oxidecomputer/console@a228b75b)
bump omicron (only one tiny diff in validators)
* [fc91ec1e](oxidecomputer/console@fc91ec1e)
oxidecomputer/console#2256
* [39b4491e](oxidecomputer/console@39b4491e)
oxidecomputer/console#2230
* [e4e912ca](oxidecomputer/console@e4e912ca)
oxidecomputer/console#2247
* [dcf09ec9](oxidecomputer/console@dcf09ec9)
oxidecomputer/console#2217
* [c36b3d63](oxidecomputer/console@c36b3d63)
oxidecomputer/console#2238
* [a8eb7745](oxidecomputer/console@a8eb7745)
oxidecomputer/console#2251
* [9b20b7c9](oxidecomputer/console@9b20b7c9)
oxidecomputer/console#2248
* [f20a5bcb](oxidecomputer/console@f20a5bcb)
oxidecomputer/console#2245
* [b815dd8f](oxidecomputer/console@b815dd8f)
oxidecomputer/console#2244
* [8c7b2946](oxidecomputer/console@8c7b2946)
add node_modules to eslint ignore patterns
* [90e78dbb](oxidecomputer/console@90e78dbb)
oxidecomputer/console#2237
* [b603d2dd](oxidecomputer/console@b603d2dd)
oxidecomputer/console#2242
* [bfce37c7](oxidecomputer/console@bfce37c7)
upgrade @oxide/openapi-gen-ts to 0.2.2
* [efceb17d](oxidecomputer/console@efceb17d)
oxidecomputer/console#2236
* [1aa46459](oxidecomputer/console@1aa46459)
oxidecomputer/console#2235
* [b400ae78](oxidecomputer/console@b400ae78)
oxidecomputer/console#2225
* [7bb3bbf7](oxidecomputer/console@7bb3bbf7)
oxidecomputer/console#2229
* [c56a9ec5](oxidecomputer/console@c56a9ec5)
oxidecomputer/console#2228
* [cd9d1f99](oxidecomputer/console@cd9d1f99)
oxidecomputer/console#2227
* [ee269bd9](oxidecomputer/console@ee269bd9)
oxidecomputer/console#2223
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.

Specify IP pool for ephemeral IP on instance create Ephemeral IP checkbox on instance create form

4 participants