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

Manage camel-cased dominant-baseline attribute in preact/compat #2859

Merged
merged 2 commits into from
Sep 15, 2021

Conversation

nmondon
Copy link
Contributor

@nmondon nmondon commented Dec 12, 2020

Hello!

What does it do?

Manage dominant-baseline attribute when written in camel case with preact/compat

Related issue

A tiny fix to manage #2858 for preact/compat.

Tests?

I also had a new unit test in render.test.js to ensure that camel-cased attributes listed in CAMEL_PROPS will be transformed... let me know if you want me to remove or modify it.

@coveralls
Copy link

coveralls commented Dec 12, 2020

Coverage Status

Coverage remained the same at 99.622% when pulling 372185e on nmondon:fix-dominant-baseline-attr into 24a859b on preactjs:master.

@nmondon nmondon changed the title Manage camel-cased dominant-baseline attribute in React compat Manage camel-cased dominant-baseline attribute in Preact compat Dec 12, 2020
@nmondon nmondon changed the title Manage camel-cased dominant-baseline attribute in Preact compat Manage camel-cased dominant-baseline attribute in preact/compat Dec 12, 2020
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.

Awesome, thanks a bunch for the PR 🙌 Anything you want to do before marking it as ready to be merged? It's currently marked as a draft, but imo we can merge it in as is 👍

@nmondon
Copy link
Contributor Author

nmondon commented Dec 14, 2020

Great! Nothing to add, I'm gonna change the stage

@nmondon nmondon marked this pull request as ready for review December 14, 2020 18:23
@@ -10,7 +10,7 @@ export const REACT_ELEMENT_TYPE =
(typeof Symbol != 'undefined' && Symbol.for && Symbol.for('react.element')) ||
0xeac7;

const CAMEL_PROPS = /^(?:accent|alignment|arabic|baseline|cap|clip(?!PathU)|color|fill|flood|font|glyph(?!R)|horiz|marker(?!H|W|U)|overline|paint|stop|strikethrough|stroke|text(?!L)|underline|unicode|units|v|vector|vert|word|writing|x(?!C))[A-Z]/;
const CAMEL_PROPS = /^(?:accent|alignment|arabic|baseline|cap|clip(?!PathU)|color|dominant|fill|flood|font|glyph(?!R)|horiz|marker(?!H|W|U)|overline|paint|stop|strikethrough|stroke|text(?!L)|underline|unicode|units|v|vector|vert|word|writing|x(?!C))[A-Z]/;
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be slightly smaller:

Suggested change
const CAMEL_PROPS = /^(?:accent|alignment|arabic|baseline|cap|clip(?!PathU)|color|dominant|fill|flood|font|glyph(?!R)|horiz|marker(?!H|W|U)|overline|paint|stop|strikethrough|stroke|text(?!L)|underline|unicode|units|v|vector|vert|word|writing|x(?!C))[A-Z]/;
const CAMEL_PROPS = /^(?:accent|alignment|arabic|[bB]aseline|cap|clip(?!PathU)|color|fill|flood|font|glyph(?!R)|horiz|marker(?!H|W|U)|overline|paint|stop|strikethrough|stroke|text(?!L)|underline|unicode|units|v|vector|vert|word|writing|x(?!C))[A-Z]/;

Copy link
Contributor Author

@nmondon nmondon Dec 16, 2020

Choose a reason for hiding this comment

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

Hi Jason, I'm not sure this will fix the transformation of dominantBaseline into dominant-baseline (or alignment-baseline) as this part of the regex only applies to the start of the string.
I see your point of targeting only Baseline$ instead of ^(dominant|alignment).

Here I presume that ^baseline was here to manage baseline-shift.
We can make an attempt of dealing with those 3 attributes (dominant-baseline, alignment-baseline and baseline-shift), something like:

([Bb]aseline management is deported at the end of the regex)

const CAMEL_PROPS = /(^(?:accent|arabic|cap|clip(?!PathU)|color|fill|flood|font|glyph(?!R)|horiz|marker(?!H|W|U)|overline|paint|stop|strikethrough|stroke|text(?!L)|underline|unicode|units|v|vector|vert|word|writing|x(?!C))[A-Z])|([bB]aseline)/

I'm gonna figure this out

Copy link
Contributor Author

@nmondon nmondon Dec 16, 2020

Choose a reason for hiding this comment

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

This seems ok for the unit test and allows to manage the 3 attributes.

Suggested change
const CAMEL_PROPS = /^(?:accent|alignment|arabic|baseline|cap|clip(?!PathU)|color|dominant|fill|flood|font|glyph(?!R)|horiz|marker(?!H|W|U)|overline|paint|stop|strikethrough|stroke|text(?!L)|underline|unicode|units|v|vector|vert|word|writing|x(?!C))[A-Z]/;
const CAMEL_PROPS = /(^(?:accent|arabic|cap|clip(?!PathU)|color|fill|flood|font|glyph(?!R)|horiz|marker(?!H|W|U)|overline|paint|stop|strikethrough|stroke|text(?!L)|underline|unicode|units|v|vector|vert|word|writing|x(?!C))[A-Z])|([bB]aseline)/;

We can even get rid of the parentheses of the second capturing group [bB]aseline...

Suggested change
const CAMEL_PROPS = /^(?:accent|alignment|arabic|baseline|cap|clip(?!PathU)|color|dominant|fill|flood|font|glyph(?!R)|horiz|marker(?!H|W|U)|overline|paint|stop|strikethrough|stroke|text(?!L)|underline|unicode|units|v|vector|vert|word|writing|x(?!C))[A-Z]/;
const CAMEL_PROPS = /(^(?:accent|arabic|cap|clip(?!PathU)|color|fill|flood|font|glyph(?!R)|horiz|marker(?!H|W|U)|overline|paint|stop|strikethrough|stroke|text(?!L)|underline|unicode|units|v|vector|vert|word|writing|x(?!C))[A-Z])|[bB]aseline/;

Let me know what you think

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.

Approving the simple version for now.

@developit developit merged commit fe3d375 into preactjs:master Sep 15, 2021
@JamesHemery
Copy link

Hi @developit !

When this will be released ? Thanks you.

@developit
Copy link
Member

@JamesHemery Should be this week or next, we are due for a patch release already.

@nmondon nmondon deleted the fix-dominant-baseline-attr branch October 5, 2021 13:00
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.

5 participants