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: issue #1024 exclude name, ensureOnCavas in SVG #1025

Merged
merged 2 commits into from May 26, 2022
Merged

fix: issue #1024 exclude name, ensureOnCavas in SVG #1025

merged 2 commits into from May 26, 2022

Conversation

cmumatt
Copy link
Contributor

@cmumatt cmumatt commented May 26, 2022

Description

Fixes issue #1024

This year we added svg-passthrough functionality, which allows unknown properties to be passed through to the SVG output. Around this time, we also renamed most internal Style properties to correspond with the SVG names so most properties could flow right through the renderer into the SVG. This approach eliminated code and make life easier for the Style developer who happens to already be familiar with SVG.

The tradeoff is that we sometimes have properties that are purely internal that we do not want to "leak" into the SVG. This is implemented as an exclusion list in AttrHelper.ts. Issue #1024 observes leakage of two internal-only properties, name and ensureOnCanvas. This PR adds these two properties to the exclusion list to elide them from the SVG output.

Implementation strategy and design decisions

Updated exclusion list in AttrHelper.ts. Updated the rendered diagram set, which now lacks the name and ensureOnCanvas properties in the SVG output. This diagram set is the current means by which we catch unintended changes to rendered properties.

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new ESLint warnings
  • I have reviewed any generated changes to the diagrams/ folder

@cmumatt cmumatt linked an issue May 26, 2022 that may be closed by this pull request
@cmumatt cmumatt changed the title Fix issue #1024 exclude name, ensureOnCavas in SVG fix: issue #1024 exclude name, ensureOnCavas in SVG May 26, 2022
@codecov
Copy link

codecov bot commented May 26, 2022

Codecov Report

Merging #1025 (51d7b20) into main (228746e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1025   +/-   ##
=======================================
  Coverage   63.57%   63.57%           
=======================================
  Files          62       62           
  Lines        7810     7810           
  Branches     1786     1786           
=======================================
  Hits         4965     4965           
  Misses       2729     2729           
  Partials      116      116           
Impacted Files Coverage Δ
packages/core/src/renderer/AttrHelper.ts 53.61% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 228746e...51d7b20. Read the comment docs.

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 26, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 51d7b20
Status: ✅  Deploy successful!
Preview URL: https://dab137ed.penrose-72l.pages.dev

View logs

@cmumatt cmumatt marked this pull request as ready for review May 26, 2022 14:48
@cmumatt cmumatt requested a review from samestep May 26, 2022 14:48
Copy link
Collaborator

@samestep samestep left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@cmumatt cmumatt merged commit ed7bb4d into main May 26, 2022
@cmumatt cmumatt deleted the fix-1024 branch May 26, 2022 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't emit Penrose-specific attributes in SVG
2 participants