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

Should we use checkbox icons from Font Awesome 4 or 5? #84

Closed
samreid opened this issue Jul 19, 2021 · 4 comments
Closed

Should we use checkbox icons from Font Awesome 4 or 5? #84

samreid opened this issue Jul 19, 2021 · 4 comments

Comments

@samreid
Copy link
Member

samreid commented Jul 19, 2021

From phetsims/expression-exchange#154 and #83, we have mostly upgraded to Font Awesome 5, but one of the last Font Awesome 4 icons we are using is the checkbox. Our general consensus is that we have generally liked the style, simplicity and consistency of using font awesome 5 for everything (though some of us prefer the Font Awesome 4 scissors). However, we are still using this font awesome 4 icon for the checkboxes:

https://fontawesome.com/v4.7/icon/check-square-o

We custom-made an empty checkbox based on that style.

Font awesome 5 provides these checkboxes:

https://fontawesome.com/v5.15/icons/check-square?style=regular
https://fontawesome.com/v5.15/icons/check-square?style=solid

https://fontawesome.com/v5.15/icons/square?style=regular

The easiest way of upgrading in master would leave a period of time where some sims have the new style and some sims have the old style.

@arouinfar said:

I didn't realize checkboxes were also FA. This is a much bigger discussion that needs to involve @kathy-phet and all relevant stakeholders, perhaps at a design meeting.

@amanda-phet said:

I agree with Amy that we need to have a larger discussion about checkboxes since those would be a major change. Since the checkbox design existed before we were using font awesome, I wasn't aware we were using font awesome for checkboxes.

@samreid
Copy link
Member Author

samreid commented Jul 19, 2021

Here's a patch that uses the FA5 checkboxes:

Index: js/Checkbox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Checkbox.js b/js/Checkbox.js
--- a/js/Checkbox.js	(revision 90def436590179e00efd80f6d55bffcd3137db2a)
+++ b/js/Checkbox.js	(date 1626727566761)
@@ -19,8 +19,9 @@
 import Path from '../../scenery/js/nodes/Path.js';
 import Rectangle from '../../scenery/js/nodes/Rectangle.js';
 import SceneryConstants from '../../scenery/js/SceneryConstants.js';
-import checkEmptySolidShape from '../../sherpa/js/fontawesome-4/checkEmptySolidShape.js';
-import checkSquareOSolidShape from '../../sherpa/js/fontawesome-4/checkSquareOSolidShape.js';
+import squareSolidShape from '../../sherpa/js/fontawesome-5/squareSolidShape.js';
+import squareRegularShape from '../../sherpa/js/fontawesome-5/squareRegularShape.js';
+import checkSquareRegularShape from '../../sherpa/js/fontawesome-5/checkSquareRegularShape.js';
 import checkboxCheckedSoundPlayer from '../../tambo/js/shared-sound-players/checkboxCheckedSoundPlayer.js';
 import checkboxUncheckedSoundPlayer from '../../tambo/js/shared-sound-players/checkboxUncheckedSoundPlayer.js';
 import EventType from '../../tandem/js/EventType.js';
@@ -30,9 +31,10 @@
 
 // constants
 const BOOLEAN_VALIDATOR = { valueType: 'boolean' };
-const SHAPE_MATRIX = Matrix3.createFromPool( 0.025, 0, 0, 0, -0.025, 0, 0, 0, 1 ); // to create a unity-scale icon
-const uncheckedShape = checkEmptySolidShape.transformed( SHAPE_MATRIX );
-const checkedShape = checkSquareOSolidShape.transformed( SHAPE_MATRIX );
+const SHAPE_MATRIX = Matrix3.createFromPool( 0.025, 0, 0, 0, 0.025, 0, 0, 0, 1 ); // to create a unity-scale icon
+const uncheckedShape = squareRegularShape.transformed( SHAPE_MATRIX );
+const checkedShape = checkSquareRegularShape.transformed( SHAPE_MATRIX );
+const backgroundShape = squareSolidShape.transformed( SHAPE_MATRIX );
 
 class Checkbox extends Node {
 
@@ -102,10 +104,10 @@
 
     // @private - Create the background.  Until we are creating our own shapes, just put a rectangle behind the font
     // awesome checkbox icons.
-    this.backgroundNode = new Rectangle( 0, -options.boxWidth, options.boxWidth * 0.95, options.boxWidth * 0.95,
-      options.boxWidth * 0.2, options.boxWidth * 0.2, {
-        fill: options.checkboxColorBackground
-      } );
+    // this.backgroundNode = new Rectangle( 0, -options.boxWidth, options.boxWidth * 0.95, options.boxWidth * 0.95,
+    //   options.boxWidth * 0.2, options.boxWidth * 0.2, {
+    //     fill: options.checkboxColorBackground
+    //   } );
 
     // @private
     this.uncheckedNode = new Path( uncheckedShape, {
@@ -120,6 +122,10 @@
       fill: options.checkboxColor
     } );
 
+    this.backgroundNode = new Path( backgroundShape, {
+      fill: options.checkboxColorBackground
+    } );
+
     // @private
     this.checkboxNode = new Node( { children: [ this.backgroundNode, this.checkedNode, this.uncheckedNode ] } );
 

image

@samreid samreid removed their assignment Jul 19, 2021
@arouinfar
Copy link

Thanks for the mockup @samreid! I think it'll really help the discussion.

@samreid
Copy link
Member Author

samreid commented Jul 19, 2021

Thanks! I was skeptical about the new one since I like the Font Awesome 4 checkbox icon so much, but in testing the Font Awesome 5 one, it also seemed good.

@samreid
Copy link
Member Author

samreid commented Jul 22, 2021

@amanda-phet: I prefer the FA4 one, it fits well with our style
@samreid: The FA5 one is crisper, but the FA4 version "pops" more and is easier to visually parse at a glance.

@amanda-phet asked if we have technical reasons to move fully away from FA4. I pointed out that many of the technical approaches from FA5 (getting rid of FontAwesomeNode, using modular strings, etc) will work equally well for FA4 and it's not a technical problem to keep around a few FA4 shapes. Likewise, we haven't had a problem with the FA4 license and don't expect to, so it seems good to keep it as it is.

We hear sometimes from other group designers (not users) that the checkboxes look like they are for a younger audience, but since it seems clear and visually parse-able, we are happy with keeping it as it is.

We agreed to keep it as it is, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants