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

Replace more font-awesome icons in views/admin area #30961

Merged
merged 4 commits into from
Aug 7, 2024

Conversation

mjankowski
Copy link
Contributor

@mjankowski mjankowski commented Jul 8, 2024

Followup on #29612 and #27780

This one handles the rest of the app/views/admin area that was not already handled. Like the previous one, I tried to use as close to a 1:1 mapping as there was, which sometimes had same name and other times did not. Also like last time, there is some small wiggle in the exact positioning.

Some before/after (not exhaustive, but representative):

Screenshot 2024-07-08 at 15 05 54 - save before Screenshot 2024-07-08 at 15 07 59 - save after
Screenshot 2024-07-08 at 15 13 04 - warning before Screenshot 2024-07-08 at 15 13 35 - warning after
Screenshot 2024-07-08 at 15 14 57 - hourglass before Screenshot 2024-07-08 at 15 15 02 - hourglass after
Screenshot 2024-07-08 at 15 18 13 - retweet before Screenshot 2024-07-08 at 15 18 32 - cycle after
Screenshot 2024-07-08 at 15 24 50 - server set before Screenshot 2024-07-08 at 15 31 36 - server set after

@mjankowski mjankowski changed the title Replace more FA icons in admin area Replace more font-awesome icons in views/admin area Jul 8, 2024
@mjankowski mjankowski added ui Front-end, design area/web interface Related to the Mastodon web interface labels Jul 11, 2024
ClearlyClaire
ClearlyClaire previously approved these changes Jul 12, 2024
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

Not a fan of the power/power_off icons for the custom emoji but I can't really think of fitting icons. Very minor alignment issue in report interface with the new skip icon.

Otherwise this looks good to me.

@renchap
Copy link
Member

renchap commented Jul 15, 2024

Alignment looks weird, especially with the hourglass icon. I am not sure where it comes from, but it should be a simple CSS fix? Should this be something done for every material icon, to match FA's behaviour?

@vmstan
Copy link
Contributor

vmstan commented Jul 16, 2024

Not a fan of the power/power_off icons for the custom emoji but I can't really think of fitting icons. Very minor alignment issue in report interface with the new skip icon.

Otherwise this looks good to me.

I just noticed in the existing fa icons they're the same "power off" -- so at least this is an improvement.

@mjankowski
Copy link
Contributor Author

Alignment looks weird, especially with the hourglass icon. I am not sure where it comes from, but it should be a simple CSS fix? Should this be something done for every material icon, to match FA's behaviour?

Yeah, presumably there is slightly different padding in the svg between some of the material vs FA icons, and that leads to some of these different alignment issues.

My plan was to attempt to get them all replaced first in a "good enough, not outright broken" way, and then do another refinements pass wherever there are alignment/padding/whatever issues.

@mjankowski
Copy link
Contributor Author

Not a fan of the power/power_off icons for the custom emoji but I can't really think of fitting icons.

There is a "Power Settings New" icon in material that is basically the same as current FA power off, but it seems like we should correct the "enable and disable are using the same icon" thing while we're in here, and there's not really an "opposite" of that one. There are "toggle on" and "toggle off" in material which are almost meaningless but at least are different than each other ... could switch to that or something else instead of power/power_off.

@vmstan
Copy link
Contributor

vmstan commented Jul 16, 2024

Not a fan of the power/power_off icons for the custom emoji but I can't really think of fitting icons.

There is a "Power Settings New" icon in material that is basically the same as current FA power off, but it seems like we should correct the "enable and disable are using the same icon" thing while we're in here, and there's not really an "opposite" of that one. There are "toggle on" and "toggle off" in material which are almost meaningless but at least are different than each other ... could switch to that or something else instead of power/power_off.

I think the toggles next to each other aren't really helpful if they don't actually toggle, as a single item individually. Just visually it doesn't make sense to me.

@mjankowski
Copy link
Contributor Author

Switched to use a radio button checked/unchecked icon instead. Strikes me as marginally better than power options, and an improvement over the current same-icon state of things.

We're not there yet - but I want to avoid bikeshedding to exact perfection here; would strongly prefer to get a "good enough" pass at replacement, and then continue to refine/replace/restyle/etc in advance of release but after we fully remove FA.

@mjankowski
Copy link
Contributor Author

Added a CSS change which improves the vertical align dynamic in the admin area table headers.

Before:

Screenshot 2024-07-22 at 10 14 11

After:

Screenshot 2024-07-22 at 10 13 58

@renchap renchap requested a review from a team July 29, 2024 16:54
@Gargron Gargron added this pull request to the merge queue Aug 7, 2024
Merged via the queue into mastodon:main with commit 84c3cc4 Aug 7, 2024
28 checks passed
@mjankowski mjankowski deleted the more-fa-icons branch August 8, 2024 13:44
justinwritescode pushed a commit to justinwritescode/mastodon that referenced this pull request Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/web interface Related to the Mastodon web interface ui Front-end, design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants