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

extends the CAMEL_PROPS regex to accept svg attribut clipPathUnits #2251

Merged

Conversation

friebe
Copy link
Contributor

@friebe friebe commented Jan 15, 2020

issue #2036

@coveralls
Copy link

coveralls commented Jan 15, 2020

Coverage Status

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

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

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
Copy link
Contributor Author

friebe commented Jan 16, 2020

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

@friebe
Copy link
Contributor Author

friebe commented Jan 16, 2020

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

@JoviDeCroock
Copy link
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

@friebe
Copy link
Contributor Author

friebe commented Jan 16, 2020

@JoviDeCroock mistake. i revert it to color.

compat/src/render.js Outdated Show resolved Hide resolved
@friebe friebe requested a review from developit January 17, 2020 14:26
Copy link
Member

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

@friebe
Copy link
Contributor Author

friebe commented Jan 20, 2020

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

@JoviDeCroock
Copy link
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.

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

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
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
Copy link
Contributor Author

friebe commented Jan 24, 2020

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
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)

Copy link
Member

@developit developit 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! 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
Copy link

pika-ci bot commented Jan 27, 2020

🚀 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.

None yet

6 participants