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

Use different icons for save/commit #102

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Use different icons for save/commit #102

wants to merge 3 commits into from

Conversation

ahuang11
Copy link
Contributor

Jim mentioned that the checkmark wasn't intuitive; not sure if this is more intuitive.

image

or

image

or
image

@ahuang11 ahuang11 requested a review from hoxbro April 24, 2024 15:42
@ahuang11 ahuang11 changed the title Use different icons Use different icons for save/commit Apr 24, 2024
Copy link

codspeed-hq bot commented Apr 24, 2024

CodSpeed Performance Report

Merging #102 will not alter performance

Comparing use_icons (33213d5) with main (3b08c28)

Summary

✅ 6 untouched benchmarks

@hoxbro
Copy link
Member

hoxbro commented Apr 24, 2024

I'm definitely not attached to the existing icons, but I don't think these are better.

@ahuang11
Copy link
Contributor Author

ahuang11 commented Apr 24, 2024

At the very least I think the database is better than a triangle. Maybe it can be a commit icon

@ahuang11
Copy link
Contributor Author

a
image

b
image

c
image

@hoxbro
Copy link
Member

hoxbro commented Apr 24, 2024

I don't think the database icon is that well-known. Maybe the save icon for upload?

holonote/app/panel.py Outdated Show resolved Hide resolved
holonote/app/panel.py Outdated Show resolved Hide resolved
@ahuang11
Copy link
Contributor Author

image

@ahuang11
Copy link
Contributor Author

ahuang11 commented Apr 24, 2024

Actually, maybe using button icons is better, and eventually, with the popups, we can remove the radiobuttongroup.

image

@hoxbro
Copy link
Member

hoxbro commented Apr 24, 2024

We should not use ButtonIcons before we can replace all buttons with them.

Edit: Maybe it is okay if we align them with the buttons.

@ahuang11
Copy link
Contributor Author

ahuang11 commented Apr 24, 2024

Okay, let's hold off on this PR for now. Thanks for the intermediate feedback; I think I have a better idea forward once the popups PR is merged:

  1. Split out delete from the RadioButtonGroup as a separate ButtonIcon that is only visible when tapped on an existing glyph
  2. Use ToggleIcon for + and edit
  3. Use ButtonIcon for the rest

@hoxbro
Copy link
Member

hoxbro commented Apr 24, 2024

Unfortunately, you can't use tap only for selection/deletion.

If we have a point-point (1d for both axes), that would also need a tap to mark.

We likely need a mode changer somewhere.

@ahuang11
Copy link
Contributor Author

I'm not sure I follow.

To clarify, I imagine tapping once on an existing glyph, then a popup shows, containing a ButtonIcon(name="delete"). Upon clicking that ButtonIcon, the selected glyph gets deleted.

@hoxbro
Copy link
Member

hoxbro commented Apr 24, 2024

If you want to make an annotation which is a single point, you would also need to use a tap stream.

@ahuang11
Copy link
Contributor Author

Still not sure I follow, but I can't seem to create a MRE of a point (and I don't think the docs show how to, unless I missed it)

#103

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.

2 participants