Skip to content

Conversation

@benjaminleonard
Copy link
Contributor

@benjaminleonard benjaminleonard commented Oct 9, 2024

Building on your PR @david-crespo

image

As you said, modal seems to work much better here. Kept it as a route so we can link to it from the instances list page. One thing to keep in mind is because it's a link, you can get here even if the instance is not currently stopped – so we should highlight that, even if the API would prevent it. Do we want to say that an instance must be stopped, failed or creating (or is this sufficient)? I doubt most people would be quick enough whilst it was creating, and if it's failed you just wouldn't see the message.

image

We can reuse the number fields from the instance create form. On that, I think we should remove the number limit and instead let the form validation describe to the user why they cannot create an instance above the CPU and memory threshold rather than just preventing it.

image

Unsure if this error handling is sufficient. Do we want to expand the modal to show errors like the side modal form, instead of the toast?

@vercel
Copy link

vercel bot commented Oct 9, 2024

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

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Oct 10, 2024 3:44pm

Handled in #2496 instead
@david-crespo
Copy link
Collaborator

Better for it not to be a link and just pop the modal on the list view when you're there, and pop it on the detail view when you're there. It might be as simple as {resizeModalOpen && <ResizeModal />} in each page with a useState controlling the modal state. I can try it soon if you don't get to it.

@benjaminleonard
Copy link
Contributor Author

Sounds good. Why do you think it's better? Simpler implementation?

@david-crespo
Copy link
Collaborator

More the UX. I think it’s weird for clicking resize on instance list to take you to a different page when it doesn’t have to.

@benjaminleonard
Copy link
Contributor Author

Ah of course, I guess I was only really looking at it from the root instance view route.

@benjaminleonard
Copy link
Contributor Author

Trying to figure out a treatment that tells you the instance name, since you can do this direct from the instances list and it might not be clear which instance is selected.

image

Similar to silo images demote:
image

image

Side modal resource:
image

image

onActivate() {
navigate(pb.instanceResize(instanceSelector))
onActivate: () => {
options.onResizeClick && options.onResizeClick(instance)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly this does not fail when run locally.

But also there must be a better way of doing this? onResizeClick should not be optional but our declaration of options as {} insists on it.

@benjaminleonard
Copy link
Contributor Author

Converted over to a regular non-route modal.

CleanShot 2024-10-10 at 12 43 18

I think that works well.

@david-crespo david-crespo merged commit c258574 into instance-resize Oct 30, 2024
8 checks passed
@david-crespo david-crespo deleted the instance-resize-modal branch October 30, 2024 17:02
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.

3 participants