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

Refactor icons: harmonize contextually, use filled status icons #4584

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

Conversation

pat-s
Copy link
Contributor

@pat-s pat-s commented Dec 18, 2024

Follow-up to #4414 which started to use the outline icon-set for the pipeline status icons.

  • Changed duration and settings icon
image
  • Changed alert and attention icons to alert-circle-outline
image
  • Changed download icon
image

Other changes

  • Changed "bandage" icon to "wrenchCogOutline"

@pat-s pat-s added ui frontend related build_pr_images If set, the CI will build images for this PR and push to Dockerhub labels Dec 18, 2024
@qwerty287

This comment was marked as off-topic.

@pat-s

This comment was marked as off-topic.

@qwerty287

This comment was marked as off-topic.

@pat-s

This comment was marked as off-topic.

@qwerty287

This comment was marked as off-topic.

@qwerty287 qwerty287 marked this pull request as ready for review December 18, 2024 19:31
@qwerty287 qwerty287 marked this pull request as draft December 18, 2024 19:32
@pat-s

This comment was marked as off-topic.

@pat-s pat-s force-pushed the feat/outline-icons branch from 59fc1e9 to f76b94b Compare December 18, 2024 19:49
@pat-s pat-s force-pushed the feat/outline-icons branch from f76b94b to e4bbceb Compare December 18, 2024 20:01
@pat-s pat-s marked this pull request as ready for review December 18, 2024 20:07
@xoxys
Copy link
Member

xoxys commented Dec 19, 2024

This is another all or nothing discussion:

  • all icons should be used from the same library
  • all semantically related icons (e.g. all pipeline status icons) should use the same style
  • NOT all used icons need to use the same style (just because pipeline status icons are outlined doesn't mean everything need to be outlined)

To me, there is no need to use ALL outlined icons whenever possible. But this time I'll leave the decision to someone else 😃 Personally, I don't like the octagon icons.

@xoxys
Copy link
Member

xoxys commented Dec 20, 2024

@pat-s what do you think about alert-circle-outline for the linter tab instead of the octagon icons?

@pat-s
Copy link
Contributor Author

pat-s commented Dec 20, 2024

Fine with me. Overall I am wondering if there should be different icons for error and warning.

@xoxys
Copy link
Member

xoxys commented Dec 20, 2024

If we stick with different colors, I don't think it's necessary. Do you have an idea for different icons?

@pat-s
Copy link
Contributor Author

pat-s commented Dec 20, 2024

Maybe mdi:stop-circle-outline in red for errors and mdi:alert-circle-outline in yellow for warnings

@pat-s
Copy link
Contributor Author

pat-s commented Dec 27, 2024

@xoxys In addition to the points discussed in chat I opted for cogOutline for the admin list panels as the adjacent icon (e.g. trash) is also outlined. Hope that is OK?

@xoxys
Copy link
Member

xoxys commented Dec 27, 2024

Wasnt the idea to harmonize the icons? Why mixing outlined and regular ones again?

@pat-s
Copy link
Contributor Author

pat-s commented Dec 27, 2024

Wasnt the idea to harmonize the icons? Why mixing outlined and regular ones again?

Harmonizing on a contextual level. Some were outlined before, e.g. trashcan. The regular trashcan looks bulky, I think you agree?

Here's a preview of what I mean:

image

-> Both outlined

All status icons are now filled. The other standalone ones (e.g. cog in the navbar) are regular.

@xoxys
Copy link
Member

xoxys commented Dec 27, 2024

Sure some were outlined before nevertheless we wanted to harmonize it.

IMO trash-can looks good and not too bulky, if we want to avoid individual preferences (as much as possible) this looks a bit random to me and could cause discussions in the future again.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 27, 2024

Hm. To me, the combined outlined icons from my previous comment look way more harmonic together than the current state:

image

If you agree with the other changes of the current state here, I'd draw my "virtual joker" and ask for this combination (both outlined as in my previous comment) to get accepted 😅️

@xoxys
Copy link
Member

xoxys commented Dec 27, 2024

Alright lets keep it this way.

@pat-s pat-s force-pushed the feat/outline-icons branch from 8d16033 to 93d8657 Compare December 27, 2024 12:00
@pat-s pat-s changed the title Use circle-outline icons consistently Refactor icons: harmonize contextually, use filled status icons Dec 27, 2024
@@ -15,7 +15,7 @@
<SvgIcon v-else-if="name === 'settings'" :path="mdiCog" size="1.3rem" />
<SvgIcon v-else-if="name === 'settings-outline'" :path="mdiCogOutline" size="1.3rem" />
<SvgIcon v-else-if="name === 'trash'" :path="mdiTrashCanOutline" size="1.3rem" />
<SvgIcon v-else-if="name === 'status-blocked'" :path="mdiPlayCircle" size="1.3rem" />
<SvgIcon v-else-if="name === 'status-blocked'" :path="mdiPlayOutline" size="1.3rem" />
Copy link
Member

Choose a reason for hiding this comment

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

This change is wrong. Status icons should now all be the filled circle.

@anbraten
Copy link
Member

anbraten commented Dec 27, 2024

Changed icon button size to w-9 (2.25 rem) from w-8 as this aligns better with the line height the icons are in.

For consistency I would suggest to stick to a 4-point / 8-point UI system. Those spacing issues often result from other parts having a wrong margin, padding or border not being aligned to it.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 27, 2024

For consistency I would suggest to stick to a 4-point / 8-point UI system. Those spacing issues often result from other parts having a wrong margin, padding or border not being aligned to it.

So you mean reduce the line-height from 1.3rem to something like 1.2? This would then likely match with h-8.

@anbraten
Copy link
Member

That would be probably the easiest here, right?

@pat-s
Copy link
Contributor Author

pat-s commented Dec 27, 2024

To match with h-8, yes. But I am not sure 1.2 is maybe a bit to narrow. Need to check.

@xoxys
Copy link
Member

xoxys commented Dec 27, 2024

But how exactly does 1.2rem (14.4pt) fit into an 8 point grid system that suggests using multiples of 8 and 4? Then we can stick to 1.3rem (15.6pt) as well, no?

@pat-s
Copy link
Contributor Author

pat-s commented Dec 27, 2024

Yeah it doesn't fit so well. I think my thought was wrong.

So now, h-8 w-8 paired with 1.3rem. Is that good for everyone?

@xoxys
Copy link
Member

xoxys commented Dec 27, 2024

While it might make sense to use w-8 h-8 instead of -9 how is this related to the default size of the icons? Changing them to 1.2rem will not make them fit with the default line height of 24px. Would just like to understand how to use the 8-point grid correctly.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 28, 2024

Can we outsource this into a discussion/chat?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build_pr_images If set, the CI will build images for this PR and push to Dockerhub ui frontend related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants