Remove requirement to specify icon class in umb-icon directive by bjarnef · Pull Request #10805 · umbraco/Umbraco-CMS

bjarnef · 2021-08-06T10:19:16Z

Prerequisites

I have added steps to test this contribution in the description below

Description

This PR is a follow-up on this PR #9255

Currently <umb-icon> use SVG icons, but also fallback to legacy font icons for now. To support both version is was required to specify icon name in class attribute as well otherwise it didn't work in case a font icon was used.

E.g. <umb-icon icon="icon-document" class="icon-document"></umb-icon>

This would be much nicer:
<umb-icon icon="icon-document"></umb-icon> or
<umb-icon icon="icon-document" class="another-class"></umb-icon>

I tried added icon name to class="umb-icon {{icon}} but it has some side effects since the directive use transclude and replace, so e.g. in editor actions menu which added -hidden class via ngClass it didn't work and then hidden error and success icons was suddently all visible.

We really only need this specific to the font/legacy icon, so instead we can wrap the  element and ensure this inherit the correct font , while not adding the pseudo classes on the parent element.

In future we might remove this part, so only SVG icons are supported (maybe in backoffce rebuild ~ v10).

Test notes

SVG icons are shown. Try with both core SVG icons and custom SVG icon in App_Plugins folder.
Example folder with coffee-cup.svg: CustomIcons.zip
Font icons are shown. Try with a font icon package like Material Design Icon Pack
Colors work on SVG icon and font icons.
When changing icon and/or color in icon picker in document/media/member type and submitting dialog, the icon + color is updated in editor header.
Polling icon is spinning when enabled.
Adding an external login provider the icon is shown correct using a font awesome icon, custom font icon or SVG icon.

umbrabot · 2021-08-06T11:00:29Z

Hi there @bjarnef, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
The automated tests all pass (see "Checks" tab on this PR)
The level of security for this contribution is the same or improved
The level of performance for this contribution is the same or improved
Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

bjarnef · 2021-08-06T11:16:44Z

@nathanwoulfe @nul800sebastiaan this would clean a lot of stuff 😁
still need some testing, but so far it still looks nice with core SVG icons, custom SVG icon and custom font icon 🗡️

bjarnef · 2021-08-06T13:01:23Z

With these changes we also need to check font icons and SVG icons can be used in actions menu: #10757

…nto v8/feature/umb-icon-svg-legacy # Conflicts: # src/Umbraco.Web.UI.Client/src/views/components/umb-mini-search.html

bjarnef · 2021-08-06T22:00:49Z

nathanwoulfe · 2021-08-06T23:36:04Z

This is awesome @bjarnef! Always bugged me that we had to pass in the same value in two places (obvs didn't bug me enough to jump in and fix it 🙄)

Will try find the time to test

bjarnef · 2021-08-07T10:43:08Z

Further it should be possible to use <umb-icon> with color classes in e.g. a custom dashboard. In have used umbraco.svg from here: https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/assets/icons/icon-umbraco.svg

<umb-icon icon="icon-umbraco" class="large color-indigo"></umb-icon>

The directive also use replace and transclude but I don't recall if is has been possible to use a custom SVG icon inside <umb-icon> e.g.

<umb-icon class="large color-cyan">
            <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 315.89 315.89">
                <path d="M0,157.74A157.95,157.95,0,1,1,158,315.89,157.95,157.95,0,0,1,0,157.74Zm154.74,54.09a155.41,155.41,0,0,1-36.5-3.29,27.92,27.92,0,0,1-19.94-16q-5.35-12.34-5.21-38.1a243,243,0,0,1,1.69-26.84q1.55-13,3.09-21.46l1.07-5.59a2,2,0,0,0,0-.49,3.2,3.2,0,0,0-2.65-3.17L75.92,93.67h-.44a3.19,3.19,0,0,0-3.11,2.48c-.35,1.31-.56,2.27-1.17,5.38-1.16,6-2.24,11.85-3.43,20.38a264.17,264.17,0,0,0-2.3,27.94,145.24,145.24,0,0,0,0,19.57q.72,25.94,8.9,41.42t27.72,22.3q19.53,6.81,54.43,6.66h2.91q34.94.15,54.41-6.66t27.71-22.3q8.17-15.53,8.91-41.42a145.24,145.24,0,0,0,0-19.57,266.84,266.84,0,0,0-2.3-27.94c-1.2-8.44-2.27-14.26-3.44-20.38-.61-3.11-.81-4.07-1.16-5.38a3.21,3.21,0,0,0-3.12-2.48h-.52l-20.38,3.18a3.2,3.2,0,0,0-2.68,3.17,4,4,0,0,0,0,.49l1.08,5.59q1.55,8.48,3.12,21.46a245.68,245.68,0,0,1,1.65,26.84q.27,25.69-5.21,38.07a27.9,27.9,0,0,1-19.76,16.07,155.19,155.19,0,0,1-36.48,3.29Z" />
            </svg>
</umb-icon>

The SVG here is the source of umbraco.svg.
It partially works, but doesn't render the SVG icon. @MMasey do you recall if this previous worked?

bjarnef · 2021-08-07T10:58:38Z

It seems it may work by adding ng-transclude here instead. We {{icon}} rendered on a parent element and/or  and  (SVG) element for a few reasons.

Sometimes value passed in to icon contains both icon and color, e.g. icon-document color-green, so it need to inherit font and color.
The second class in icon property is not always a color, e.g. in logviewer polling the second class is fa-spin, where it need to inherit CSS animation.

<span aria-hidden="true" class="umb-icon">
    <span class="umb-icon__inner {{icon}}" ng-transclude>
        <span ng-if="svgString" ng-bind-html="svgString"></span>
        <i ng-if="!svgString && icon" class="{{icon}}"></i>
    </span>
</span>

One thing I noticed doesn't work is when using a font icon inside umb-icon because ng-transclude also replace the {{icon}} class on umb-icon__inner so the font family isn't inherited.

<umb-icon icon="icon-gmd-face" class="large color-blue"></umb-icon>

<umb-icon class="large color-green">
     <i class="icon-gmd-face"></i>
</umb-icon>

This may be a option instead:

<span aria-hidden="true" class="umb-icon">
    <span class="umb-icon__inner {{icon}}">
        <ng-transclude>
            <span ng-if="svgString" ng-bind-html="svgString"></span>
            <i ng-if="!svgString && icon" class="{{icon}}"></i>
        </ng-transclude>
    </span>
</span>

bjarnef · 2021-08-07T11:48:19Z

I now have the following:

<umb-icon icon="icon-umbraco" class="large color-indigo"></umb-icon>

<umb-icon class="large color-cyan">
    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 315.89 315.89">
        <path d="M0,157.74A157.95,157.95,0,1,1,158,315.89,157.95,157.95,0,0,1,0,157.74Zm154.74,54.09a155.41,155.41,0,0,1-36.5-3.29,27.92,27.92,0,0,1-19.94-16q-5.35-12.34-5.21-38.1a243,243,0,0,1,1.69-26.84q1.55-13,3.09-21.46l1.07-5.59a2,2,0,0,0,0-.49,3.2,3.2,0,0,0-2.65-3.17L75.92,93.67h-.44a3.19,3.19,0,0,0-3.11,2.48c-.35,1.31-.56,2.27-1.17,5.38-1.16,6-2.24,11.85-3.43,20.38a264.17,264.17,0,0,0-2.3,27.94,145.24,145.24,0,0,0,0,19.57q.72,25.94,8.9,41.42t27.72,22.3q19.53,6.81,54.43,6.66h2.91q34.94.15,54.41-6.66t27.71-22.3q8.17-15.53,8.91-41.42a145.24,145.24,0,0,0,0-19.57,266.84,266.84,0,0,0-2.3-27.94c-1.2-8.44-2.27-14.26-3.44-20.38-.61-3.11-.81-4.07-1.16-5.38a3.21,3.21,0,0,0-3.12-2.48h-.52l-20.38,3.18a3.2,3.2,0,0,0-2.68,3.17,4,4,0,0,0,0,.49l1.08,5.59q1.55,8.48,3.12,21.46a245.68,245.68,0,0,1,1.65,26.84q.27,25.69-5.21,38.07a27.9,27.9,0,0,1-19.76,16.07,155.19,155.19,0,0,1-36.48,3.29Z" />
    </svg>
</umb-icon>

<umb-icon icon="icon-gmd-face" class="color-blue" style="font-size: 2.5rem;"></umb-icon>

<umb-icon class="color-green" style="font-size: 2.5rem;">
    <i class="icon-gmd-face"></i>
</umb-icon>

The polling animation is still working:

bjarnef · 2021-08-08T13:18:14Z

@nathanwoulfe I decided the replace pretty much all icons represented as  using <umb-icon> instead, so icons are handle in same way.

I also found some issues with directives/components which were self-closing e.g. <localize /> which should be <localize></localize>.

Furthermore I found propertyJoinSeparator added here 188cc2a but it wasn't used anywhere, so I have removed this. In case it was intended to be used, I guess it only should contain a separator like ,.

I tried to use <umb-icon> inside <umb-checkmark> component https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/views/components/umb-checkmark.html but it seems to cause issues with isolated scopes, so I have reverted that for now.

bjarnef · 2021-08-08T13:26:56Z

Okay, it seems it works anyway with a minor adjustment: 3a171a2

…r" in _flexbox.less

bjarnef · 2021-08-09T12:43:02Z

I also adjusted the icon used for external login providers. It would be great to test this still is shown correct in login screen / user overlay when using a font awesome icon, custom font icon or custom SVG icon.
https://our.umbraco.com/documentation/Reference/Security/auto-linking

It should be working similar to what has already been added in v9 😎
#10675

nathanwoulfe · 2021-08-11T00:57:41Z

@bjarnef that's a stack of files 😄 All looks to be working nicely - the different icon implementations shown below all work as expected, so it's nothing if not flexible!

I made one trivial change in the Examine dashboard, the icons had a class that has no corresponding CSS, so I removed that and shifted the margin-top utility to the parent element, so the icon is only receiving the icon attribute. I'll let the tests run again, just in case, but assuming that's all green I'm more than happy to merge this one.

I now have the following:

<umb-icon icon="icon-umbraco" class="large color-indigo"></umb-icon>

<umb-icon class="large color-cyan">
    <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 315.89 315.89">
        <path d="M0,157.74A157.95,157.95,0,1,1,158,315.89,157.95,157.95,0,0,1,0,157.74Zm154.74,54.09a155.41,155.41,0,0,1-36.5-3.29,27.92,27.92,0,0,1-19.94-16q-5.35-12.34-5.21-38.1a243,243,0,0,1,1.69-26.84q1.55-13,3.09-21.46l1.07-5.59a2,2,0,0,0,0-.49,3.2,3.2,0,0,0-2.65-3.17L75.92,93.67h-.44a3.19,3.19,0,0,0-3.11,2.48c-.35,1.31-.56,2.27-1.17,5.38-1.16,6-2.24,11.85-3.43,20.38a264.17,264.17,0,0,0-2.3,27.94,145.24,145.24,0,0,0,0,19.57q.72,25.94,8.9,41.42t27.72,22.3q19.53,6.81,54.43,6.66h2.91q34.94.15,54.41-6.66t27.71-22.3q8.17-15.53,8.91-41.42a145.24,145.24,0,0,0,0-19.57,266.84,266.84,0,0,0-2.3-27.94c-1.2-8.44-2.27-14.26-3.44-20.38-.61-3.11-.81-4.07-1.16-5.38a3.21,3.21,0,0,0-3.12-2.48h-.52l-20.38,3.18a3.2,3.2,0,0,0-2.68,3.17,4,4,0,0,0,0,.49l1.08,5.59q1.55,8.48,3.12,21.46a245.68,245.68,0,0,1,1.65,26.84q.27,25.69-5.21,38.07a27.9,27.9,0,0,1-19.76,16.07,155.19,155.19,0,0,1-36.48,3.29Z" />
    </svg>
</umb-icon>

<umb-icon icon="icon-gmd-face" class="color-blue" style="font-size: 2.5rem;"></umb-icon>

<umb-icon class="color-green" style="font-size: 2.5rem;">
    <i class="icon-gmd-face"></i>
</umb-icon>

The polling animation is still working:

Wrap legacy font icon so we can inherit font from parent element

0977b7b

Remove icon name from class attributes

1bb28f0

bjarnef marked this pull request as draft August 6, 2021 11:03

bjarnef added 3 commits August 6, 2021 13:08

Add back class as color is part of icon

a528ecd

Replace icon classes

622e7d8

Replace icon class

acfcef3

Use umb-icon

a69c4f8

bjarnef added 4 commits August 6, 2021 22:09

Merge branch 'v8/contrib' of https://github.com/umbraco/Umbraco-CMS i…

a6615fc

…nto v8/feature/umb-icon-svg-legacy # Conflicts: # src/Umbraco.Web.UI.Client/src/views/components/umb-mini-search.html

Remove icon classes

be2e0cc

Remove icon classes

588f602

Update color for SVG icons

d6811b0

bjarnef marked this pull request as ready for review August 6, 2021 22:06

bjarnef added 5 commits August 7, 2021 00:37

Inherit animation

15d096e

Add icon add class to SVG as well

86ab063

Update umb-icon inner wrapper

2e5128c

Fix issue with search icon styling in property editors dialog

875be0d

Formatting

a4e3b01

Adjust umb-icon with transclude

2073bef

bjarnef added 4 commits August 7, 2021 13:56

Change back to original back icon from font awesome

c787517

Adjust styling for font icon

64ea212

Replace with umb-icon

1acc2af

Use umb-icon in packager

290ce07

bjarnef added 7 commits August 8, 2021 14:40

Replace more icons

2341851

Custom HTML element should have end-closing tag

43d2f52

Replace icons with umb-icon

60f7221

Replace icons

7aac2d7

Remove unused variable

53b01a9

Replace icon

6b1995b

Replace icons

d28922e

bjarnef added 2 commits August 8, 2021 15:19

Revert to element as it was causing issues with isolated scopes

883de27

Adjust umb-checkmark to work with umb-icon

3a171a2

bjarnef added 2 commits August 8, 2021 15:37

Adjust lightbox styling

17fc6e3

Remove unused helper class and it is already available as "self-cente…

1279478

…r" in _flexbox.less

bjarnef mentioned this pull request Aug 10, 2021

Update styles on editor-header icon picker #10822

Closed

1 task

nathanwoulfe added 2 commits August 11, 2021 09:56

Merge branch 'v8/contrib' into pr/10805

0393034

remove unused class from icon, shift margin to parent element

23d9d9c

nathanwoulfe approved these changes Aug 11, 2021

View reviewed changes

nathanwoulfe merged commit 690a631 into umbraco:v8/contrib Aug 11, 2021

bjarnef deleted the v8/feature/umb-icon-svg-legacy branch August 11, 2021 06:22

nul800sebastiaan added category/dx Developer experience community/pr release/8.17.0 type/feature labels Aug 11, 2021

bjarnef referenced this pull request Aug 13, 2021

show composition icon on inherited tabs

deb21e9

nathanwoulfe mentioned this pull request Aug 17, 2021

V8: fix umb-form-check icon z-index to bring icon to top #10872

Closed

1 task

This was referenced Aug 18, 2021

Inherit text color from parent element #10894

Merged

Adjust styling of umb-file-icon #10895

Merged

bjarnef mentioned this pull request Sep 1, 2021

Use umb-icon component for icons in content type groups and tabs #11012

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove requirement to specify icon class in umb-icon directive#10805

Remove requirement to specify icon class in umb-icon directive#10805
nathanwoulfe merged 47 commits intoumbraco:v8/contribfrom
bjarnef:v8/feature/umb-icon-svg-legacy

bjarnef commented Aug 6, 2021 •

edited

Loading

Uh oh!

umbrabot commented Aug 6, 2021 •

edited by nathanwoulfe

Loading

Uh oh!

bjarnef commented Aug 6, 2021

Uh oh!

bjarnef commented Aug 6, 2021

Uh oh!

bjarnef commented Aug 6, 2021 •

edited

Loading

Uh oh!

nathanwoulfe commented Aug 6, 2021

Uh oh!

bjarnef commented Aug 7, 2021 •

edited

Loading

Uh oh!

bjarnef commented Aug 7, 2021 •

edited

Loading

Uh oh!

bjarnef commented Aug 7, 2021

Uh oh!

bjarnef commented Aug 8, 2021 •

edited

Loading

Uh oh!

bjarnef commented Aug 8, 2021

Uh oh!

bjarnef commented Aug 9, 2021

Uh oh!

nathanwoulfe commented Aug 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

bjarnef commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Test notes

Uh oh!

umbrabot commented Aug 6, 2021 • edited by nathanwoulfe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjarnef commented Aug 6, 2021

Uh oh!

bjarnef commented Aug 6, 2021

Uh oh!

bjarnef commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nathanwoulfe commented Aug 6, 2021

Uh oh!

bjarnef commented Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjarnef commented Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjarnef commented Aug 7, 2021

Uh oh!

bjarnef commented Aug 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjarnef commented Aug 8, 2021

Uh oh!

bjarnef commented Aug 9, 2021

Uh oh!

nathanwoulfe commented Aug 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bjarnef commented Aug 6, 2021 •

edited

Loading

umbrabot commented Aug 6, 2021 •

edited by nathanwoulfe

Loading

bjarnef commented Aug 6, 2021 •

edited

Loading

bjarnef commented Aug 7, 2021 •

edited

Loading

bjarnef commented Aug 7, 2021 •

edited

Loading

bjarnef commented Aug 8, 2021 •

edited

Loading