Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instance action buttons #2508

Merged
merged 15 commits into from
Oct 30, 2024
Merged

Instance action buttons #2508

merged 15 commits into from
Oct 30, 2024

Conversation

benjaminleonard
Copy link
Contributor

I plan to be fussy about it
— David Crespo

image

Partially fixes #2419 (just the instance detail page)

Copy link

vercel bot commented Oct 17, 2024

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

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Oct 30, 2024 2:24pm

@david-crespo
Copy link
Collaborator

Couple of things I noticed while trying it out. These weren't happening with the menu because typically the menu will be closed while the action is running unless you rush to open it back up.

Tooltip is kinda weird

The first thing I noticed was that the disabled cursor overlaps the tooltip in an annoying way. So I wonder if we should just forget the cursor thing. It wasn't noticeable in the menu because the tooltips appear to the left instead of underneath.

Then I started changing the window size and noticed that the tooltip starts showing up on the left if the window gets wide enough. I don't know why that is yet but it's kind of silly. Maybe it should even be on top. Not sure.

2024-10-17-button-tooltip.mp4

Lack of state change feedback near the buttons, plus tooltip is even weirder than I thought

Now we click the button. Several weird things here.

  • Because start has no confirmation step, you still have your mouse over the start button after clicking it, and because you can't start a starting instance, the button immediately goes disabled and the tooltip shows up saying you can't start it. It feels like an 🙅 error in response to your click, which the opposite of what we want to convey — it's only because your click succeeded that you're now getting this error message. Bizarre.
    • Possible fix: add a confirmation to start? Stupid, yet smart.
  • Stop has a different problem. Stop similarly is disabled immediately after the stop goes through, but because there's a confirmation, you at least are not on the button when it goes disabled. But it still feels weird because start doesn't get enabled until the instance is done stopping. So there's a weird little stretch there where both buttons are disabled and there's no feedback happening.
    • Possible fix: while the instance is stopping, put a spinner in the stop button? While the instance is starting, but a spinner in the start button?
2024-10-17-start-stop-states.mp4

How it looks with spinners in the buttons

I can't say I like it. It's weird as hell.

2024-10-17-spinners-in-buttons.mp4

@benjaminleonard
Copy link
Contributor Author

benjaminleonard commented Oct 29, 2024

The first thing I noticed was that the disabled cursor overlaps the tooltip in an annoying way. So I wonder if we should just forget the cursor thing. It wasn't noticeable in the menu because the tooltips appear to the left instead of underneath.

Happy to skip the cursor. It can be a little aggressive.

Then I started changing the window size and noticed that the tooltip starts showing up on the left if the window gets wide enough. I don't know why that is yet but it's kind of silly. Maybe it should even be on top. Not sure.

autoPlacement({
  allowedPlacements: ['top', 'bottom'],
});

This might be better? Restricting its position to either top or bottom, not left or right.

Possible fix: add a confirmation to start? Stupid, yet smart.

I think this is a good idea. Having it start directly with the button feels like it's missing a little bit of necessary friction. The modal is also a place we can include information if we feel like we want to describe some stuff around utilization.

Stop has a different problem. Stop similarly is disabled immediately after the stop goes through, but because there's a confirmation, you at least are not on the button when it goes disabled.

I don't actually mind that. Since we do have a little spinner on the instance status in the properties table directly.

I dislike spinners in the buttons here because it ocludes the button text – also what happens if an instance gets stuck in starting or stopping.

@benjaminleonard
Copy link
Contributor Author

Actually allowedPlacements is not required if we just include placement top or bottom.

@david-crespo
Copy link
Collaborator

Yep, I like it. The confirm on start does fix most of it — now having both buttons disabled for a couple seconds doesn't really feel weird. Maybe disabled:cursor-default on the button? I find it a little weird to have cursor-pointer on a disabled button.

This list is a little overwhelming. Currently we populate it using the full list of actual states, but I would be ok with fudging it and manually excluding a couple, saying either Only running or failed... or maybe even just Only running instances can be stopped.

@benjaminleonard
Copy link
Contributor Author

Only running instances can be stopped sounds good to me.

Agreed on cursor-default. We should follow up elsewhere in another PR if we want to remove cursor-not-allowed completely.

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.

Beautiful.

@david-crespo david-crespo enabled auto-merge (squash) October 30, 2024 14:24
@david-crespo david-crespo merged commit 2382425 into main Oct 30, 2024
8 checks passed
@david-crespo david-crespo deleted the instance-action-buttons branch October 30, 2024 14:33
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.

Make resource actions on detail page more discoverable
2 participants