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

Canvas: Avoid conflicting stylesheets when loading SVG icons #74461

Merged
merged 13 commits into from Sep 18, 2023

Conversation

adela-almasan
Copy link
Contributor

While investigating https://github.com/grafana/support-escalations/issues/7316 we noticed that the SVGs used had the same class names defined between the <style> tag, which was causing the SVGs to borrow styling from one to another. The issue doesn't happen for SVGs that don't have custom styling defined.
The solution was to add a unique ID to each SVG and use the ID when defining the classes.

Before

svgs_before.mov

After

svgs_after.mov

Fixes https://github.com/grafana/support-escalations/issues/7316

Special notes for your reviewer:
Cases covered:

  • add new ID to SVG when not defined
  • if ID is defined, use it/overwrite the existing one in <style>

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@adela-almasan adela-almasan added area/panel/canvas Issues related to canvas panel no-changelog Skip including change in changelog/release notes backport v10.1.x labels Sep 6, 2023
@adela-almasan adela-almasan added this to the 10.2.x milestone Sep 6, 2023
@adela-almasan adela-almasan requested a review from a team as a code owner September 6, 2023 15:07
@adela-almasan adela-almasan self-assigned this Sep 6, 2023
@adela-almasan adela-almasan requested a review from a team as a code owner September 6, 2023 15:07
@adela-almasan adela-almasan requested review from L-M-K-B and eledobleefe and removed request for a team September 6, 2023 15:07
@ryantxu ryantxu changed the title Canvas: Fix conflicting SVG classes Canvas: Avoid conflicting stylesheets when loading SVG icons Sep 6, 2023
@ryantxu ryantxu added add to changelog and removed no-changelog Skip including change in changelog/release notes labels Sep 6, 2023
@adela-almasan
Copy link
Contributor Author

tested in Geomap and it doesn't seem to be affected, the markers are keeping their colors, as their should. However, I can't use the SVGs from the escalation, not sure if there's something wrong with them - but they don't work regardless of my changes. @drew08t any idea?

geomap_svg.mov

@adela-almasan adela-almasan enabled auto-merge (squash) September 6, 2023 19:28
Copy link
Contributor

@nmarrs nmarrs left a comment

Choose a reason for hiding this comment

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

A few nits - overall looks good and tested locally 💯

public/app/core/components/SVG/utils.ts Outdated Show resolved Hide resolved
public/app/core/components/SVG/utils.ts Outdated Show resolved Hide resolved
public/app/core/components/SVG/SanitizedSVG.test.tsx Outdated Show resolved Hide resolved
@grafana grafana deleted a comment from grafana-delivery-bot bot Sep 18, 2023
@adela-almasan adela-almasan enabled auto-merge (squash) September 18, 2023 17:12
@adela-almasan adela-almasan merged commit 7171b35 into main Sep 18, 2023
15 checks passed
@adela-almasan adela-almasan deleted the esc7316_svg_icon branch September 18, 2023 17:38
grafana-delivery-bot bot pushed a commit that referenced this pull request Sep 18, 2023
@adela-almasan adela-almasan mentioned this pull request Oct 10, 2023
20 tasks
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
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.

None yet

4 participants