Skip to content

extends the CAMEL_PROPS regex to accept svg attribut clipPathUnits#2251

Merged
JoviDeCroock merged 9 commits into
preactjs:masterfrom
friebe:bugfix/extend_normalization-regex
Jan 27, 2020
Merged

extends the CAMEL_PROPS regex to accept svg attribut clipPathUnits#2251
JoviDeCroock merged 9 commits into
preactjs:masterfrom
friebe:bugfix/extend_normalization-regex

Conversation

@friebe

@friebe friebe commented Jan 15, 2020

Copy link
Copy Markdown
Contributor

issue #2036

@coveralls

coveralls commented Jan 15, 2020

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 99.807% when pulling b74938e on friebe:bugfix/extend_normalization-regex into a6cab5b on preactjs:master.

Comment thread compat/src/render.js Outdated

@JoviDeCroock JoviDeCroock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey, thanks for the PR could you add a test?

@friebe

friebe commented Jan 16, 2020

Copy link
Copy Markdown
Contributor Author

@JoviDeCroock Shall i build a new test file for it or shall i extend the existing test/svg.test.js file ?

@friebe

friebe commented Jan 16, 2020

Copy link
Copy Markdown
Contributor Author

Oh man i need help. Why are now the checks and the Pica CI failed ?

@JoviDeCroock

Copy link
Copy Markdown
Member
  svg
    ✖ should render SVG to DOM #2
      HeadlessChrome 73.0.3683 (Linux 0.0.0)
    AssertionError: expected '<svg viewBox="0 0 100 100"><text textAnchor="mid">foo</text><path d="M 0 0 L 100 100" vectorEffect="non-scaling-stroke"></path></svg>' to equal '<svg viewBox="0 0 100 100"><text text-anchor="mid">foo</text><path d="M 0 0 L 100 100" vector-effect="non-scaling-stroke"></path></svg>'
      + expected - actual
      -<svg viewBox="0 0 100 100"><text textAnchor="mid">foo</text><path d="M 0 0 L 100 100" vectorEffect="non-scaling-stroke"></path></svg>
      +<svg viewBox="0 0 100 100"><text text-anchor="mid">foo</text><path d="M 0 0 L 100 100" vector-effect="non-scaling-stroke"></path></svg>

Failing test and failing coverage

Comment thread compat/test/browser/svg.test.js Outdated
Comment thread compat/test/browser/svg.test.js Outdated
@friebe

friebe commented Jan 16, 2020

Copy link
Copy Markdown
Contributor Author

@JoviDeCroock mistake. i revert it to color.

Comment thread compat/src/render.js Outdated
@friebe friebe requested a review from developit January 17, 2020 14:26

@JoviDeCroock JoviDeCroock left a comment

Copy link
Copy Markdown
Member

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 for contributing this!

@friebe

friebe commented Jan 20, 2020

Copy link
Copy Markdown
Contributor Author

That sounds good to me @JoviDeCroock. What are my next steps to close this PR ?

@JoviDeCroock

Copy link
Copy Markdown
Member

@friebe I'll wait for @marvinhagemeister or @developit to also give it an approval.

I see you have disabled the edit from maintainers this allows us to ensure your branch is up to date with master. That means that if we get it approved we'll probably ask you to set your branch up to date with master by rebasing or merging master into this branch.

@marvinhagemeister marvinhagemeister left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sweet 👍 Looks good to me 💯

@JoviDeCroock

Copy link
Copy Markdown
Member

Hey @friebe

When you get time could you update your branch or give us the right to do so by ticking "allow edit from maintainers"

@friebe

friebe commented Jan 24, 2020

Copy link
Copy Markdown
Contributor Author

What does it mean check "compressed Size / build" fail.
Even the details of the error do not give me any information to understand the situation.

@JoviDeCroock

Copy link
Copy Markdown
Member

It can't access your branch, pinging @developit (you have a blocker with requested changes and the compressed-size plugin can't access forked repo's)

@developit developit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! sorry for the slow response.

(aside: the size bot no longer fails PRs when it can't comment, it'll be a while before Github has a fix for the fork permissions thing)

@JoviDeCroock JoviDeCroock merged commit aca606a into preactjs:master Jan 27, 2020
@pika-ci

pika-ci Bot commented Jan 27, 2020

Copy link
Copy Markdown

🚀 This PR has been merged! Once a new release is created, any changes will become available on npm. Until then, you can load and install it directly from the Pika CDN:

npm install https://cdn.pika.dev/preact/master

porfirioribeiro pushed a commit to porfirioribeiro/preact that referenced this pull request Feb 3, 2020
…reactjs#2251)

* extends the CAMEL_PROPS regex to accept svg attribut clipPathUnits

* remove unnecessary trailing pipe in CAMEL_PROPS regex

* add test for svg attribute (clipPathUnits) manipulation

* adjust CAMEL_PROPS regex to passes tests

* refactor svg.test to manipulation attributePathUnits

* remove "clip" from CAMEL_PROPS regex

Co-authored-by: Jason Miller <developit@users.noreply.github.com>
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.

6 participants