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

fix: use lumo iconset with vaadin-icon #2475

Merged
merged 1 commit into from
Sep 8, 2021

Conversation

sissbruecker
Copy link
Contributor

Description

Currently it is not possible to use the Lumo iconset with the vaadin-icon component that was introduced in 21:

<vaadin-icon icon="lumo:bar-chart"></vaadin-icon> <!-- doesn't work -->

The issue is that the Lumo iconset is currently not loaded as a vaadin-iconset, which means that vaadin-icon can not find the icon collection.

To fix this I included the vaadin-iconset.js of the Lumo theme into the all imports file (via icons.js). That also means that both, the iron-iconset and vaadin-iconset versions of the Lumo iconset will be loaded, increasing the payload transmitted to the browser. As I understand it that is intended to allow backwards compatibility with iron-icon in the meantime.

Also not 100% in which versions this fix should land. For now I targeted master and 21.

Type of change

  • Bugfix

@sonarcloud
Copy link

sonarcloud bot commented Sep 8, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sissbruecker sissbruecker merged commit 9d0be40 into master Sep 8, 2021
@sissbruecker sissbruecker deleted the fix/use_vaadin_icon_with_lumo_icons branch September 8, 2021 10:28
web-padawan pushed a commit that referenced this pull request Sep 8, 2021
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 22.0.0.alpha4 and is also targeting the upcoming stable 22.0.0 version.

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

Successfully merging this pull request may close these issues.

3 participants