Skip to content

Replaced <i> with umb-icon directive in languages overview#9557

Merged
nathanwoulfe merged 2 commits intoumbraco:v8/contribfrom
poornimanayar:feature/umb-icon-language-overview
Jan 27, 2021
Merged

Replaced <i> with umb-icon directive in languages overview#9557
nathanwoulfe merged 2 commits intoumbraco:v8/contribfrom
poornimanayar:feature/umb-icon-language-overview

Conversation

@poornimanayar
Copy link
Copy Markdown
Contributor

Hi,

I have replaced <i> with an umb-icon in the languages overview screen

image

Poornima

@poornimanayar poornimanayar changed the title replaced <i> with umb-icon directive in languages overview Replaced <i> with umb-icon directive in languages overview Dec 16, 2020
Copy link
Copy Markdown
Contributor

@nathanwoulfe nathanwoulfe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @poornimanayar

@nathanwoulfe nathanwoulfe merged commit 1c65662 into umbraco:v8/contrib Jan 27, 2021
@nathanwoulfe nathanwoulfe added category/ui User interface community/pr release/8.12.0 release/no-notes Not directly part of the release (updating README, build scripts, tests, etc.) labels Jan 27, 2021
@bjarnef
Copy link
Copy Markdown
Contributor

bjarnef commented Jan 27, 2021

@nathanwoulfe for now we still need icon name in class attribute. In this case is is probably only in core, but would potentially break if you overwrite the icon-globe with a custom font icon. #9255

@c9mb
Copy link
Copy Markdown

c9mb commented Jan 27, 2021

I guess one question is - are icons really semantic content or presentation - i.e. do they belong in the html at all ?

@nathanwoulfe
Copy link
Copy Markdown
Contributor

@bjarnef for all uses? I thought that was only the case where an SVG version didn't exist?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category/ui User interface community/pr release/no-notes Not directly part of the release (updating README, build scripts, tests, etc.) release/8.12.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants