-
Notifications
You must be signed in to change notification settings - Fork 8
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 convert from FA4 icons to FA5 icons? #83
Comments
I ran a quick (not exhaustive) search to find Font Awesome 5 icons that seemed like a close match (in name at least) to Font Awesome 4, here is the result. Leftmost column is font awesome 4. Columns to the right are options from Font Awesome 5. For the ones with missing icons, I suspect there are alternatives, but perhaps they have different names. I'm seeing some icons that don't seem as nice in the Font Awesome 5, such as the scissors and the checkmark in the box (but I have recollection that we manually edited something about the checkmark in the box). If we have to keep around the Font Awesome 4 infrastructure, there may be diminishing returns on moving individual occurrences to 5. Still, it seem worth running past a designer to see if we want to choose any of the Font Awesome 5 icons to replace the Font Awesome 4 icons. @arouinfar can you please take a look and make recommendations? |
That certainly seems like the case. Here are a few examples:
In general, I prefer Font Awesome 5 and would be fine with it being propagated to all sims. The only exception would be for the checkboxes, so it would be good to know if we are using Font Awesome or something different for that. The older scissors do look nice, but the newer scissors are more consistent with other icons. The scissors alone are not enough reason to keep Font Awesome 4 around, in my opinion. |
In the commit, I converted Blackbody Spectrum from Font Awesome 4 to Font Awesome 5. The main change was transforming the old code: content: new FontAwesomeNode( 'camera', { maxWidth: BUTTON_ICON_WIDTH } ), to the new code: content: new Camera( { maxWidth: BUTTON_ICON_WIDTH, fill: 'black' } ), There are 3 other occurrences of camera:
Once those are converted, we can remove the camera data from sherpa/js/fontawesome-4/FontAwesomeIcons4_5.js Adding to developer meeting to review this problem, and determine how to proceed. Should it be a chip away issue? Should each developer address their own sims, or should a central person make most of the changes and assign to responsible devs for review only? |
This issue can be closed when all side issues for conversion have created. |
There are 70 usages of FontAwesomeNode across 26 repos (one is an UPDATE: What about one issue per responsible dev? And let's go one-by-one so we can iron out problems as we go instead of everyone running into the same problems at the same time. Perhaps I should start, then send it over to someone else. Here's a quick list, which was easy to generate since responsible devs is machine-readable since https://github.com/phetsims/special-ops/issues/185. I should also mention that it was very convenient to have everything in one file. @samreid
@jonathanolson @zepumph |
I only had two, so I just created my own issues in my own repos. |
Before we start converting, several "code review" things:
class FANode extends Path {
/**
* @param {string} svgPath
* @param {Object} [options]
*/
constructor( svgPath, options ) {
assert && assert( typeof svgPathData === 'string' );
const shape = new Shape( svgPath );
super( shape, options );
// @public (read-only)
this.svgPath = svgPath;
this.shape = shape;
}
} Subclasses would look like: const SVG_PATH = '...';
class Camera extends FANode {
/**
* @param {Object} [options}
*/
constructor( options ) {
super( SVG_PATH, options );
}
}
export default Camera;
If you choose not to make the above changes:
Assigning to @samreid for response. |
4/22/21 dev meeting: @samreid hasn't had a chance to review my feedback. Porting to FA5 is on hold. |
I’d also like the name to reflect the style, so that there are no collisions if importing regular/AddressBook and solid/AddressBook. How should we name them? Some possibilities:
What do you recommend? |
The migration is going really well. There are a few occurrences left. @jonathanolson can you please do the remaining joist ones? I wasn't sure how to test them easily.
After these are done, please reassign to @samreid to delete FontAwesomeNode, font-awesome-4-5.html and FontAwesomeIcons4_5.js. We will need to keep checkEmptySolidShape.js and checkSquareOSolidShape.js for now since it is used by checkboxes. |
I have handled the case for geometric optics. |
I've taken care of expression-exchange and projectile-motion. I left "sun" unchecked above because I'm not sure what we're going to do about the checkboxes. @samreid said above:
Have we looked at |
I completed phetsims/joist#707, and just did a global search for FontAwesomeNode. The only spot left besides minor doc updates is:
|
Thanks to everyone for pulling together on this issue. I've removed FontAwesomeNode and its icons. The last thing is about the checkboxes which will be decided in #84. sherpa/font-awesome-4 looks like it has many usages in website things, so it cannot be deleted at this time. Closing. |
From #81, I'd like to compare the FA5 icons to FA4. If FA5 icons are equally good or better, it would simplify our project to eliminate the FA4 approach. The first step will be to show a before/after comparing each icon. If the changes are nontrivial, then we will need to ask for designer recommendation. If we need to keep any of the FA4 icons, let's consider wrapping them in an approach like the FA5 approach, for simplicity.
The text was updated successfully, but these errors were encountered: