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

feat(ux): improve gc #1420

Merged
merged 7 commits into from
Apr 21, 2020
Merged

feat(ux): improve gc #1420

merged 7 commits into from
Apr 21, 2020

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Apr 18, 2020

As @RubenKelevra noted, GC might take a while to finish running. So I took their suggestions and implemented them (see bellow).

This PR:

  • Improves UX while GC is running;
  • Adds a custom error message when GC fails;
  • Cleans up tray logic a bit.

License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com

@RubenKelevra
Copy link
Contributor

It's neat for blocking this option, but we should show it more prominently.

How about changing the green dot to a yellow broom, and instead of "IPFS is running" change the message to "IPFS is running the garbage collector" or something like this? 🤔

I think the ususally degraded performance justifies that the icon should be yellow instead of green.

@hacdias
Copy link
Member Author

hacdias commented Apr 18, 2020

@RubenKelevra that's definitely a good idea! Let me see what I can do. I actually wanted to clean up tray logic. Might be an opportunity to improve it a bit.

@hacdias
Copy link
Member Author

hacdias commented Apr 18, 2020

GC not running:

Screenshot 2020-04-18 at 09 29 15

GC running:

image

Edit: I renamed GC to Garbage Collector.

@RubenKelevra
Copy link
Contributor

Well, I would still vote for a broom instead of just 'yellow' which looks like something is partly broken 🤔

But the rest looks very nice!

@hacdias
Copy link
Member Author

hacdias commented Apr 18, 2020

Ahah I like the Broom idea but we'd need to add that icon at some specific sizes and I think it's easier for now to keep the yellow circle. 🧹🧹

@hacdias hacdias mentioned this pull request Apr 18, 2020
22 tasks
@hacdias hacdias changed the title feat: improve gc ux feat(ux): improve gc Apr 18, 2020
@RubenKelevra
Copy link
Contributor

Ahah I like the Broom idea but we'd need to add that icon at some specific sizes and I think it's easier for now to keep the yellow circle.

Here you go:

https://commons.wikimedia.org/wiki/File:Artworkbean_broom.svg

@hacdias
Copy link
Member Author

hacdias commented Apr 18, 2020

Thanks! Let's see what @lidel and @jessicaschilling have to say and save the icon for a next iteration. Let's not forget that the icon will be displayed in a very small size so it can't be too detailed.

@jessicaschilling
Copy link
Contributor

This is cool! I’m a little concerned that users will think IPFS isn’t running when GC is running. There isn’t a great option for text due to space but maybe “Running with Garbage Collection”? And I’d gray out the stop and restart items rather than remove them entirely, to indicate you can still stop and restart, just not right now.

An aside: We should capitalize “IPFS is Running” to match the other menu items.

Regarding broom - it’s going to be really tough to make that legible at small sizes, unfortunately. We’re also working on some stuff in ipfs-css that’s nearly finished, so suggest leaving that out for the time being.

Copy link
Contributor

@jessicaschilling jessicaschilling left a comment

Choose a reason for hiding this comment

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

See my thoughts earlier 😊

@hacdias
Copy link
Member Author

hacdias commented Apr 18, 2020

I’m a little concerned that users will think IPFS isn’t running when GC is running. There isn’t a great option for text due to space but maybe “Running with Garbage Collection”?

What if we explain in the dialog before? I'm concerned about such a long sentence because it would seem all the other options are really small compared to that one.

An aside: We should capitalize “IPFS is Running” to match the other menu items.

Will do!

Regarding broom - it’s going to be really tough to make that legible at small sizes, unfortunately.

👍

@jessicaschilling
Copy link
Contributor

“Running (Garbage Collecting)”?
If it takes a long time, they might forget about the dialog 😉

@hacdias
Copy link
Member Author

hacdias commented Apr 18, 2020

image

Like this? Seems a bit displaced 😢

@jessicaschilling
Copy link
Contributor

I see what you mean, but I do think it’s better than being unclear. Plus it’s not a super-frequent scenario. Let’s see what @lidel says.

@hacdias
Copy link
Member Author

hacdias commented Apr 18, 2020

Yeah, I agree. Let's wait for lidel input!

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
assets/locales/en.json Outdated Show resolved Hide resolved
hacdias and others added 2 commits April 20, 2020 20:32
Co-Authored-By: Marcin Rataj <lidel@lidel.org>
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

LGTM (small typo below).
Logic in updateMenu is slowly getting messy, but tolerable for now.

@hacdias remember to merge this before #1411 (and #1411 should be rebased on top of this)

assets/locales/en.json Outdated Show resolved Hide resolved
@RubenKelevra
Copy link
Contributor

Regarding broom - it’s going to be really tough to make that legible at small sizes, unfortunately.
Maybe doing a small animation would help?

Here's some inspiration :)

source

Co-Authored-By: Marcin Rataj <lidel@lidel.org>
@hacdias hacdias merged commit dbbd955 into master Apr 21, 2020
@hacdias hacdias deleted the feat/improve-ux-gc branch April 21, 2020 07:05
@lidel lidel mentioned this pull request May 4, 2020
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