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

Add a few existing icons to menus #1335

Merged
merged 1 commit into from
Feb 2, 2022
Merged

Add a few existing icons to menus #1335

merged 1 commit into from
Feb 2, 2022

Conversation

thiagowfx
Copy link
Contributor

Do you follow the guidelines?

Notes:

  • Only added pre-existing icons from sprite.svg. If desired, we could incorporate more icons from https://tabler-icons.io/, but opted to keep it simple / minimal for now.
  • Only added the unambiguous items. Some examples are ambiguous, for example: ( ) Mark this page as read vs ( ) Mark all as read. There is a "mark all as read" icon already, however it could be deemed confusing to add both of them in the same context for two different actions, as the actions are very similar. I leave this open for debate.
  • If you think there are more places to add existing icons to I could've missed, feel free to point them out.

Sample view attached below.
127 0 0 1_8080_feed_1_entries (1)

@fguillot
Copy link
Member

Each menu links should have an icon for consistency. New icons could be added to sprite.svg. It could be tricky to find an icon for each action.

Only added the unambiguous items. Some examples are ambiguous, for example: ( ) Mark this page as read vs ( ) Mark all as read. There is a "mark all as read" icon already, however it could be deemed confusing to add both of them in the same context for two different actions, as the actions are very similar. I leave this open for debate.

Open to suggestions if there are better icons on https://tabler-icons.io/

@thiagowfx
Copy link
Contributor Author

I like your idea to distinguish between "mark" versus "mark all". Will send an updated patch in a few days.

@thiagowfx
Copy link
Contributor Author

thiagowfx commented Jan 29, 2022

Added the two aforementioned icons. Missing, with some ideas:

I'll wait for your feedback before adding more icons.

@fguillot
Copy link
Member

@thiagowfx
Copy link
Contributor Author

Also added:

At this point there are so many icons it was hard to keep track of them all with individual commits; I've gone ahead and squashed everything.

@fguillot
Copy link
Member

fguillot commented Feb 1, 2022

Actually, the category page (/categories) already had icons for feeds and articles. So, icon-news and icon-file-text should not be necessary, icon-feeds and icon-entries should be used instead. Sorry about that, I forgot about it in my last comment.

image

On more thing, in the file sprite.svg, it would be better to name the icons based on their usage in the application. For example, instead of having id="icon-eye", it could be id="icon-show-all-entries, id="icon-folder-plus" -> id="icon-add-category", etc. This way, the SVG content could be replaced easily if another icon is more suitable.

@thiagowfx
Copy link
Contributor Author

thiagowfx commented Feb 1, 2022

Sorry about that, I forgot about it in my last comment.

No problem, fixed (in the same commit).

it would be better to name the icons based on their usage in the application

Did this in a separate commit for ease of review and to possibly spot typos, we could squash it once you're happy with it.

For reference:

ack -i 'icon.*check' -l | xargs sed -i -e 's/{{ icon "check" }}/{{ icon "mark-page-as-read" }}/g' &&\
ack -i 'icon.*checks' -l | xargs sed -i -e 's/{{ icon "checks" }}/{{ icon "mark-all-as-read" }}/g' &&\
ack -i 'icon.*eye' -l | xargs sed -i -e 's/{{ icon "eye" }}/{{ icon "show-all-entries" }}/g' &&\
ack -i 'icon.*eye-off' -l | xargs sed -i -e 's/{{ icon "eye-off" }}/{{ icon "show-unread-entries" }}/g' &&\
ack -i 'icon.*folder-plus' -l | xargs sed -i -e 's/{{ icon "folder-plus" }}/{{ icon "add-category" }}/g' &&\
ack -i 'icon.*plus' -l | xargs sed -i -e 's/{{ icon "plus" }}/{{ icon "add-feed" }}/g' &&\
ack -i 'icon.*file-import' -l | xargs sed -i -e 's/{{ icon "file-import" }}/{{ icon "feed-import" }}/g' &&\
ack -i 'icon.*file-export' -l | xargs sed -i -e 's/{{ icon "file-export" }}/{{ icon "feed-export" }}/g' &&\
ack -i 'icon.*folders' -l | xargs sed -i -e 's/{{ icon "folders" }}/{{ icon "categories" }}/g'

@fguillot
Copy link
Member

fguillot commented Feb 2, 2022

LGTM. This page seems to have a missing icon: /category/create.

@thiagowfx
Copy link
Contributor Author

thiagowfx commented Feb 2, 2022

Thanks, added. Also squashed the commits.

@fguillot fguillot merged commit c891ab2 into miniflux:master Feb 2, 2022
@thiagowfx thiagowfx deleted the icons branch February 2, 2022 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants