Daemonite / discourse-material-theme

Material Design for Discourse
MIT License
21 stars 28 forks source link

Some icons are not translated properly #17

Open amotl opened 5 years ago

amotl commented 5 years ago

Introduction

We just tried to get your recent amendments from #3, #13 and #14 on board but obviously this wouldn't help as the Grunt task has not been run. No hurries on that one, we are already very happy with the current state and will just continue reporting things we come across - even minor ones you might have recognized already - and hope you are okay with that.

Preface

We just ran a full Discourse rebuild from the command line and recognized a subtle regression barely able to spot between the version running before, which was installed a few days ago and the current version v2.2.0.beta8 +37.

Problem

The fa-eye icon designated to signal the state of a topic item being listed or unlisted in the topic list on the right hand side just disappeared.

With Default theme

image

With Material theme

image

amotl commented 5 years ago

Investigation

The HTML code currently rendered is

<i class="fa fa-far-eye-slash d-icon d-icon-far-eye-slash unlisted"></i>

Seeing the new FA naming scheme slip through there, this might be coming from the Font Awesome transition, however we haven't been able to spot any specific commit in Discourse coming in recently.

Workaround

Amending this to

<i class="fa fa-eye-slash d-icon d-icon-fa-eye-slash unlisted"></i>

made the icon appear again.

This snippet of code added to the end of the function faClasses applies this specific amendment to all icons in question.

// Fix MBT#17: Hack to translate class names like "fa-far-eye-slash" to "fa-eye-slash" again
classNames = classNames.replace('far-', 'fa-').replace('fa-fa-', 'fa-');

Hope this helps.

sesemaya commented 5 years ago

This is very likely related to Discourse switching to Font Awesome 5 SVG icons.

There is a workaround for this in common/head_tag.html and it may need to be updated.

We'll try to get our Discourse updated and then have a look at this issue.

amotl commented 5 years ago

After the most recent update, the "edit"- and "delete"-post icons disappeared. To mitigate this, we added

// Fix MBT#17: Translate "-alt" icons to non-"-alt"
classNames = classNames.replace('-alt', '');

at the end of the Font Awesome icons support function "faClasses".

We also found that fa fa-clock (used for "Set Topic Timer") is missing completely.

ryanmstokes commented 5 years ago

How can I fix this please? Ive tried to add this line to the function in the head file but it makes no difference.

amotl commented 5 years ago

Dear Maya,

  1. We can confirm the fa-eye icon works with the most recent version: image

  2. The "edit"- and "delete"-post icons are also working flawlessly: image

  3. The fa-clock icon used for "Set Topic Timer" is still missing: image

However, this is really a minor issue, so this is just for the records.

Thanks again for your awesome work on this theme!

With kind regards, Andreas.

ryanmstokes commented 5 years ago

It is still broken for me, the edit and delete icons are missing, the checkbox icons on surveys are missing, how do I fix this please?

amotl commented 5 years ago

Dear @ryanmstokes,

you might want to apply the workaround as outlined above for a quick fix. For doing so, you will find the function faClasses at https://discourse.example.org/admin/customize/themes/22/common/head_tag/edit when replacing the URL parts appropriately with the corresponding ones of your installation or just navigating to "Admin » Customize » Themes » Daemonite Material » Edit CSS/HTML » Common » \</head>".

There, you might want to insert classNames = classNames.replace('-alt', ''); right before return classNames;, like that:

  // Support for Font Awesome icons
  function faClasses(icon, params) {
    let classNames = `fa fa-${icon.replacementId || icon.id} d-icon d-icon-${
      icon.id
    }`;

    if (params) {
      if (params.modifier) {
        classNames += ' fa-' + params.modifier;
      }

      if (params['class']) {
        classNames += ' ' + params['class'];
      }
    }

    // Fix MBT#17: Translate "-alt" icons to non-"-alt" ones to
    // counter disappearing "edit"- and "delete"-post icons.
    // https://github.com/Daemonite/discourse-material-theme/issues/17#issuecomment-479716834
    classNames = classNames.replace('-alt', '');

    return classNames;
  }

Please also share the Discourse version you are running on with us and check whether you updated the theme to the most recent version.

Asking that, I would like to mention that it already works perfectly for us without that workaround after the last upgrade. Now, we are running Discourse v2.3.0.beta6 +148, probably with the most recent version of discourse-material-theme.

With kind regards, Andreas.

svrcore commented 5 years ago

I have this issue too, my version is 2.2.4.

image
amotl commented 5 years ago

Dear @svrcore, did this issue resolve for you? Otherwise, you might want to try the hotfix as outlined above which worked around the issue on our installation the other day.