Skip to content

Conversation

@david-crespo
Copy link
Collaborator

@david-crespo david-crespo commented Apr 24, 2024

The diff is a nightmare compared to the amount of changes because it's a lot of shuffling and fiddly margin stuff. I'd focus on checking the rendered result.

https://console-git-firewall-rules-help-oxidecomputer.vercel.app/projects/mock-project/vpcs/mock-vpc

image image  image

@vercel
Copy link

vercel bot commented Apr 24, 2024

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

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

<div className="flex flex-col gap-3">
<TextField
name="portRange"
label="Port filters"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to add a prop that lets me tweak the styling on this label to match the others. Or just use the inner components directly, but that’s fiddly too.

@benjaminleonard
Copy link
Contributor

Some of those notes could possible be moved to the help text / description directly on the input.

Whilst you're in here, some placeholder values would be wonderful.

@david-crespo
Copy link
Collaborator Author

I don't think I can squeeze the help into individual fields because it's really about the whole thing. Placeholders are also kind of tough because they depend on which type you have picked in the listbox.

@charliepark
Copy link
Contributor

This might be better for a different PR, but since we're making this sparkle …

would it be possible to disable the overall form's submit button while the input fields (as here) have un-saved content?
Screenshot 2024-04-24 at 9 47 54 AM
As it is, it's easy to not realize that you haven't actually "saved" the sub-form, and it won't make it into the final firewall rule.

I think this has come up in conversation before, but I'm not sure if we've ever discussed it in an issue. If not relevant for this PR, no worries, and I can create a new issue. If it fits, though, great.

@david-crespo
Copy link
Collaborator Author

That's a cool idea. I'll see if there's a way to make it work.

@david-crespo
Copy link
Collaborator Author

What you might be thinking of that came up before was making Enter submit the mini form when you're in one of those fields rather than submitting the whole form, we fixed that.

@david-crespo
Copy link
Collaborator Author

david-crespo commented Apr 24, 2024

I don't think I can make it make sense to block leftover stuff in the input. You wouldn't want to just disable submit, you'd want it to be a validation error you can go find, but you'd only want it to be triggered by submit. We'd have to get pretty elaborate and I don't think it's worth it.

I added validation for port range because it's easy, but for the target and host fields, it's a lot more complicated because the way you validate the value input depends on which type is selected in the listbox. That's captured in #2187.

@david-crespo
Copy link
Collaborator Author

I'm going to merge this as-is and we can redo the text boxes in a different style as a followup per design feedback.

@david-crespo david-crespo merged commit 2e7e4a0 into main Apr 24, 2024
@david-crespo david-crespo deleted the firewall-rules-help branch April 24, 2024 20:16
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.

4 participants