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 fontawesome icons in AppBar #108

Merged
merged 18 commits into from
Dec 9, 2024
Merged

💄 📦 use fontawesome icons in AppBar #108

merged 18 commits into from
Dec 9, 2024

Conversation

jdhoffa
Copy link
Member

@jdhoffa jdhoffa commented Dec 3, 2024

  • 📦 gains fontawesome dependency
  • use fontawesome icons instead of buttons

Before

Screenshot 2024-12-03 at 15 24 35

After

Screenshot 2024-12-03 at 15 27 36

Towards #91

@jdhoffa jdhoffa requested a review from MonikaFu as a code owner December 3, 2024 14:27
@jdhoffa jdhoffa changed the title 💄 use fontawesome icons in AppBar 📦 use fontawesome icons in AppBar Dec 3, 2024
@jdhoffa
Copy link
Member Author

jdhoffa commented Dec 3, 2024

I will wait for @MonikaFu 's review here, as this PR adds a new dependency (and also is a totally superfluous style change, so not urgent)

@jdhoffa jdhoffa requested a review from AlexAxthelm December 4, 2024 12:20
Copy link

github-actions bot commented Dec 4, 2024

Docker build status

commit_time git_sha image
2024-12-08T22:07:49Z 51fbef5 ghcr.io/rmi-pacta/pacta-dashboard-svelte:pr-108

@MonikaFu
Copy link
Contributor

MonikaFu commented Dec 4, 2024

Not sure if it is a good change. Maybe for GitHub it is but not sure if the save is clear enough as an icon to be honest.

@jdhoffa jdhoffa mentioned this pull request Dec 4, 2024
3 tasks
@jdhoffa
Copy link
Member Author

jdhoffa commented Dec 4, 2024

Not sure if it is a good change. Maybe for GitHub it is but not sure if the save is clear enough as an icon to be honest.

Yup totally fair enough. I thought it looked more elegant than the buttons, but perhaps also less user friendly.

With that said, I imagine if a user was curious they would just click the icon, and relatively quickly understand that it saves a data_archive ¯_(ツ)_/¯

@jdhoffa
Copy link
Member Author

jdhoffa commented Dec 4, 2024

I do feel that the icons look more professional though...

Before

Screenshot 2024-12-04 at 22 32 18

After

Screenshot 2024-12-04 at 22 31 40

@jdhoffa jdhoffa changed the title 📦 use fontawesome icons in AppBar 🎨 📦 use fontawesome icons in AppBar Dec 5, 2024
@jdhoffa
Copy link
Member Author

jdhoffa commented Dec 5, 2024

I added a hover effect which could be a compromise (although I'm also not convinced of this):
Screenshot 2024-12-05 at 12 29 40

(Sorry the screenshot is sort of bogus)

@jdhoffa jdhoffa changed the title 🎨 📦 use fontawesome icons in AppBar 💄 📦 use fontawesome icons in AppBar Dec 5, 2024
@jdhoffa
Copy link
Member Author

jdhoffa commented Dec 5, 2024

@MonikaFu FYI, a call with @hodie lead to the following feedback:

  • The icons look more professional than the buttons
  • The buttons/ text is more user-friendly than the icons
  • Both features in question (GitHub, downloading an archive of the data) are likely reserved for pretty advanced users either way

A general comment was also made:
These icons probably shouldn't be in the AppBar, but rather in a footer of the dashboard. They should exist and be available to advanced users who know what they want/ know that the buttons are there, but don't need to be front and center.

Overall actions then are:

@hodie
Copy link

hodie commented Dec 6, 2024

@MonikaFu FYI, a call with @hodie lead to the following feedback:

  • The icons look more professional than the buttons
  • The buttons/ text is more user-friendly than the icons
  • Both features in question (GitHub, downloading an archive of the data) are likely reserved for pretty advanced users either way

A general comment was also made: These icons probably shouldn't be in the AppBar, but rather in a footer of the dashboard. They should exist and be available to advanced users who know what they want/ know that the buttons are there, but don't need to be front and center.

Overall actions then are:

Just one clarification: the conclusion was to have icons.

If/when we have a proper bottom bar (like e.g. here on GitHub) there should be a text for GitHub just as there should be for Terms, Security, Contact. The download action probably does make sense to have on top since it's an action about the content.

But the whole top bar will need to be reviewed once we can test it end-to-end and the dashboard is embedded into another page. Seems like a top-bar and especially the RMI logo will be confusing in that context.

Copy link

github-actions bot commented Dec 6, 2024

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-12-09 12:51 UTC

@jdhoffa
Copy link
Member Author

jdhoffa commented Dec 6, 2024

FYI @MonikaFu I removed the hover over effect cause it was clunky and distracting, but this is ready for review now.

Copy link
Contributor

@MonikaFu MonikaFu left a comment

Choose a reason for hiding this comment

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

Adding a title to the icons results in a small banner appearing when they hover over the icon for longer. I think it adds a bit to the user friendliness without sacrificing the lean look of the icons.

src/routes/+layout.svelte Show resolved Hide resolved
src/routes/+layout.svelte Outdated Show resolved Hide resolved
src/routes/+layout.svelte Outdated Show resolved Hide resolved
jdhoffa and others added 3 commits December 8, 2024 12:12
@jdhoffa jdhoffa requested a review from MonikaFu December 8, 2024 22:08
Copy link
Contributor

@MonikaFu MonikaFu left a comment

Choose a reason for hiding this comment

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

LGTM

@jdhoffa jdhoffa merged commit 8f88a29 into main Dec 9, 2024
11 checks passed
@jdhoffa jdhoffa deleted the 29-add_icons branch December 9, 2024 12:51
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