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

Fa5 compatibility #23

Closed
wants to merge 2 commits into from
Closed

Conversation

lavolp3
Copy link
Contributor

@lavolp3 lavolp3 commented Jan 6, 2019

Arrows symbol did not work anymore in new MM version.
Changing class prefix to "fas" ("fa" is deprecated in FA5) and name to arrows-alt-h (free for non-pro FA5 users) got them back again.
Can not yet see any other symbol disappearing from this action

ianperrin and others added 2 commits November 25, 2018 16:05
Include virtual activities in chart
Arrows symbol did not work anymore in new MM version.
Changing class prefix to "fas" ("fa" is deprecated in FA5) and name to arrows-alt-h (free for non-pro FA5 users) got them back again.
Can not yet see any other symbol disappearing from this.
@ianperrin
Copy link
Owner

Hi @lavolp3

Thanks for the contribution. Much appreciated.

It looks like v2.6.0 has introduced a bug with introducing support of FA5.

If you remove any module which references FA5 (e.g. the calendar module), then the arrows symbol appears. Including these updated modules causes the arrow symbol doesn't appear.

Your pull request goes someway to fixing the issue, providing that

  1. MagicMirror v2.6.0 is installed
  2. Another module which references FA5 is included in config.js

You can replicate this by applying the changes to the module installed in an older version of MagicMirror or modifying the config.js file so that it only includes MMM-Strava. In both these cases, all icons do not appear.

To fix this,

  1. The minimum version of MagicMirror required by the module must be specified. e.g. by adding the following above L13 in MMM-Strava: requiresVersion: "2.6.0",
  2. FontAwesome5 must be referenced in MMM-Strava's getStyles function. It looks like the MagicMirror documentation for using FA5 has not been updated since v2.6.0 was released last week. However, based on the changes made to the default calendar module when FA5 was added, line 44 of MMM-Strava.js should be amended to read return ['font-awesome5.css', 'MMM-Strava.css']; Note: I don't believe 'font-awesome5.v4shims.css', is required as you have change the class from fa to fas - this eliminating the need for FA4 compatibility within this module, but this needs testing.

If you can make and test these changes, I can merge the pull. In the meantime, I'll raise a bug to see if it can be fixed

Thanks again

@lavolp3
Copy link
Contributor Author

lavolp3 commented Jan 7, 2019

Understood. So it's a bit more complicated. Do I understand right, that some module or process determines that MM uses FA5 and not FA4?
Then when you only use MMM-Strava, FA4 is used and my amendment does not work.
Interesting.
I'll work on it.
I heard that MM was going to use FA5 but was of the impression that it was backwards compatible.

@lavolp3
Copy link
Contributor Author

lavolp3 commented Jan 7, 2019

can't we just include "font-awesome5.v4shims.css" into the getStyles part as well and keep everything else as before?

@ianperrin
Copy link
Owner

Do I understand right, that some module or process determines that MM uses FA5 and not FA4?

The core MagicMirror code only includes third party (vendor) stylesheets (css) files specified by the modules contained in config.js. One benefit of this is to reduced page load times.

A module specifies which third party (vendor) stylesheets it requires within the getStyles function (see developer documentation for more info).

The issues you have identified appears to be caused by the fact one module (the default calendar) specifies 'font-awesome5.css' and a second (MMM-Strava) specifies 'font-awesome.css' (or FA4).

As a result, the generated HTML includes links to both the FA5 and FA4 stylesheets and conflicting behaviour is experienced.

Furthermore, changing the order of the modules in config.js appears to have an impact on behaviour too.

I heard that MM was going to use FA5 but was of the impression that it was backwards compatible.

That certainly is implied in the release notes. From what you've uncovered, it would suggest this is in fact breaking behaviour.

can't we just include "font-awesome5.v4shims.css" into the getStyles part as well and keep everything else as before?

Since your pull request changes the styles from fa fa-arrows-h to fas fa-arrows-alt-h the module no longer depends on FA4. Therefore, I don't believe adding 'font-awesome5.v4shims.css' is necessary. However, please do test both combinations on line 44 of MMM-Strava.js

@ianperrin
Copy link
Owner

@lavolp3 - I've submitted a pull request to the core MM code which should fix the issue with multiple versions of FA requested by modules.

Once this is released (in version 2.7 hopefully ), the distance icon will reappear and this pull request will not be required

Thanks

@ianperrin
Copy link
Owner

I'm closing this pull request now that version 2.7 has been released which contains the fix for the font awesome issue (see MagicMirrorOrg/MagicMirror#1525)

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

Successfully merging this pull request may close these issues.

2 participants