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

Font Awesome icons not working after v2.6.0 upgrade #1522

Closed
ianperrin opened this issue Jan 7, 2019 · 7 comments
Closed

Font Awesome icons not working after v2.6.0 upgrade #1522

ianperrin opened this issue Jan 7, 2019 · 7 comments

Comments

@ianperrin
Copy link
Contributor

ianperrin commented Jan 7, 2019

Hi @MichMich

Great work as ever getting another update out - hope you managed to celebrate the New Year whilst completing the release!

I have uncovered an issue with the Font Awesome 5 support introduced in v2.6.0

MagicMirror Version: v2.6.0

Description:
When amodule includes "fontawesome.css" (i.e. font awesome 4) and another module (e.g. the default calendar) includes "font-awesome5.css", "font-awesome5.v4shims.css" then unexpected results can occur depending on which order they are included in config.js.

Steps to Reproduce:
A: Modify config.js file in a v2.6.0 install so it only includes definitions for the default calendar module and then MMM-Strava (in that order). In this instance fa-hashtag and fa-trophy display as expected, but fa-arrows (a renamed icon) does not.

B: Modify config.js file in a v2.6.0 install so it only includes definitions for the default calendar module and then MMM-Strava (in that order). Change the default config for calendar to use symbol: "volleyball-ball". In this instance fa-hashtag and fa-trophy display as expected, but fa-arrows (a renamed icon) and fa-volleyball-ball (a new icon) do not.

C: Change config.js to include definitions for MMM-Strava then the default calendar module (in that order) and all icons appear.

D: Change config.js to omit the default calendar module (i.e. only include MMM-Strava) and all icons appear.

Expected Results:
All fa4 and fa5 icons should appear in all instances

Actual Results:
If the fa5 (e.g. calendar) module is omitted from config.js, the fa4 module works as expected.
If the fa5 (e.g. calendar) module is added to config.js before the fa4 module, the config for the fa5 module cannot use new fa5 icons AND the fa4 module cannot use renamed icons

Additional Notes:
The release notes for v2.6.0 suggest the new FA5 functionality is backwards compatible. However, it feels like this is either a bug or a breaking change.

A workaround is to release an update to the fa4 module so that any icons are renamed per the font awesome 4 upgrade notes. However, to do so means the third party module will depend on v2.6.0 (i.e. requiredVersion must be increased to 2.6.0) thus preventing existing users who choose not to upgrade from taking advantage of improvements to a module.

Also, if another fa4 module is added to the config after the upgraded fa5 modules, the problem recurs.

@ianperrin
Copy link
Contributor Author

ianperrin commented Jan 8, 2019

The issue can be replicated outside of MagicMirror e.g. FA4 css first and fa5 css first

Whilst this shows this issues is in itself not a MagicMirror bug, it is triggered by MagicMirror allowing both versions of Font Awesome stylesheets to be included in the generated html.

Furthermore, it doesn’t look like either the end user (via config.js) or module developer (via getStyles) can resolve the issue.

@MichMich
Copy link
Collaborator

MichMich commented Jan 8, 2019

This is a tough one. We might want to add a generic config which sets the desired FA-version. The only issue is that a module can't depend on either one of the libs being available.

Any recommendations in a solution?

@ianperrin
Copy link
Contributor Author

Quite a challenge indeed!

On the assumption that the goal is to adopt FA5 as the single FA library within MM and given FA do not recommend running FA4 and FA5 side by side in one project the path I would recommend is

  1. Revert the change to support FA5 as soon as possible.
  2. Notify the user and developer base that FA4 will be replaced with FA5 in version 2.7(?). In this version (and all 2.6.x releases), references to “fontawesome.css” in getStyles should include the FA5 css file (all.min.css) and v4-shim css (v4.shims.min.css) for backwards compatibility.
  3. Provide a time frame (say in version 2.8?) by which module developers should adopt FA5 fully. After which point backwards compatibility (provided by v4-shim) will be removed.

All other options seem to rely on vendor specific work arounds within the core MM code which, depending on users configuration and developer support could trigger a variety of outcomes and support headaches.

I appreciate introducing such a roadmap presents some problems, but it does seem the cleanest way forward.

The main challenge is how to force the inclusion of the v4-shim if a module references to “fontawesome.css” in getStyles without requiring changes to the module code. i.e. include two css files for one getStyles reference.

@MichMich
Copy link
Collaborator

MichMich commented Jan 8, 2019

I understand your approach, but I don't want to introduce a roadmap with breaking changes. Simplicity is key for MagicMirror².

We can probably support both FA versions by namespacing them to specific classes. If a module has the fa4 class the fa4 lib will be used, if the module has the fa5 class, the fa5 lib will be used (or something to that extend). Since both libs are just a bunch of classes, we should be able to make them work only for children of specific parent classes.

@ianperrin
Copy link
Contributor Author

ianperrin commented Jan 9, 2019

Totally support the principle and will continue searching for solutions.

I guess I was put off by the official statement and conversations such as this one on stackoverflow citing similar issues with WordPress plugins.

To avoid (minimise) breaking changes, is it possible to move to step 2 (FA5 + v4 shim) now, but not plan for step 3?

Quick tests with a modified vendor file where “fontawesome.css” installs fa5, a dummy plugin which getstyles for the fa4 shim included last in the config and reverting getstyles in the default calendar module seem to work.

This gives backwards compatibility and leaves the adoption of fa5 in the hands of plugin devs based on how they wish to support the new prefixes (fas, fab, far etc)

@MichMich
Copy link
Collaborator

MichMich commented Jan 9, 2019

A shim sounds perfect. Feel free to send a PR.

MichMich added a commit that referenced this issue Jan 10, 2019
Fix conflict between font awesome versions as per #1522
@MichMich
Copy link
Collaborator

Merged!

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