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 NavigationBarSoundToggleButton #730

Closed
pixelzoom opened this issue Jul 9, 2021 · 2 comments
Closed

Use fontawesome icons in NavigationBarSoundToggleButton #730

pixelzoom opened this issue Jul 9, 2021 · 2 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 9, 2021

Noticed while working on phetsims/sherpa#83.

I understand why the navigation bar isn’t using SoundToggleButton — it must be a joist button, hence NavigationBarSoundToggleButton.

But is there a reason why NavigationBarSoundToggleButton isn’t using the fontawesome icons that are used by SoundToggleButton, and is instead drawing complicated shapes that are almost identical to the fontawesome icons? This creates 2 issues: inconsistency of sound icons, and having to maintain complicated shapes.

@chrisklus
Copy link
Contributor

I tracked down the commit that replaced the font-awesome icons and the associated issue, from phetsims/tambo#19 (comment):

@jbphet and I worked on drawing out the sounds icon with code instead of using the font awesome icon since it was 4 MB.

In the process, we addressed what hadn't yet been done on the code end to match @arouinfar's screen shot from above, including vertically center aligning the icon with the 'P' and evenly spacing out the icons horizontally. I also slightly decreased the stroke of the X lines and curves to better match the outline of the keyboard. Here's what has currently been pushed:

image

@jbphet and I also discussed the possibility of removing the fill from the sound speaker so that it better matches the keyboard and doesn't draw the eye as much. I can discuss this idea and the fine tuning details of the above changes with @arouinfar. Preview of what this could look like:

image

Sounds like the two reasons were:

  1. the FA icon was an extra 4 MB that was nice to cut (no idea if this is still the case or if that size is an issue)
  2. We had the control to stroke the speaker and sound lines to match the keyboard outline. This appears to still be in the same state, though i'm unsure how desirable this still is compared to FA (I personally prefer the shape of this speaker). Sounds like a question for designers.

From BASE master:

image

@chrisklus chrisklus removed their assignment Jul 15, 2021
@jbphet
Copy link
Contributor

jbphet commented Jul 15, 2021

@pixelzoom said:

But is there a reason why NavigationBarSoundToggleButton isn’t using the fontawesome icons that are used by SoundToggleButton[?]

@chrisklus has described the reasons above (thanks for looking at this @chrisklus). I've added a comment with an explanation to the file header. @pixelzoom - Since you opened the issue, please review the response. If you think it's satisfactory, this can be closed. If not, please assign to a designer to decide if we should revisit.

@jbphet jbphet assigned pixelzoom and unassigned jbphet Jul 15, 2021
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

No branches or pull requests

3 participants