Skip to content

refactor(utilities): refactoring Utilities constants #1003

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

Merged
merged 5 commits into from
Aug 9, 2023

Conversation

ssi02014
Copy link
Contributor

@ssi02014 ssi02014 commented Aug 9, 2023

What is the motivation for this pull request?

Hello 👋, @remarkablemark
I worked on refactoring by isolating code that could be managed by constants inside the utilities.js.
Please review this pull request to see if it's appropriate. 🙏

What is the current behavior?

There is no change in behavior.

What is the new behavior?

There is no change in behavior.

Checklist:

Comment on lines +48 to +69

var RESERVED_SVG_MATHML_ELEMENTS = new Set([
'annotation-xml',
'color-profile',
'font-face',
'font-face-src',
'font-face-uri',
'font-face-format',
'font-face-name',
'missing-glyph'
]);

function isCustomComponent(tagName, props) {
if (tagName.indexOf('-') === -1) {
return props && typeof props.is === 'string';
}

switch (tagName) {
// These are reserved SVG and MathML elements.
// We don't mind this whitelist too much because we expect it to never grow.
// The alternative is to track the namespace in a few places which is convoluted.
// https://w3c.github.io/webcomponents/spec/custom/#custom-elements-core-concepts
case 'annotation-xml':
case 'color-profile':
case 'font-face':
case 'font-face-src':
case 'font-face-uri':
case 'font-face-format':
case 'font-face-name':
case 'missing-glyph':
return false;
default:
return true;
// These are reserved SVG and MathML elements.
// We don't mind this whitelist too much because we expect it to never grow.
// The alternative is to track the namespace in a few places which is convoluted.
// https://w3c.github.io/webcomponents/spec/custom/#custom-elements-core-concepts
if (RESERVED_SVG_MATHML_ELEMENTS.has(tagName)) {
return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I worked similarly to the existing "elementsWithNoTextChildren" approach. 🙏

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #1003 (76d08f3) into master (eaa5c38) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1003   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          177       169    -8     
  Branches        59        50    -9     
=========================================
- Hits           177       169    -8     
Files Changed Coverage Δ
lib/utilities.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@remarkablemark
Copy link
Owner

@ssi02014 I noticed you added a fix 9ec5ba1 so I wanted to confirm if that's true since it will trigger a patch version release for this package

@ssi02014
Copy link
Contributor Author

ssi02014 commented Aug 9, 2023

@remarkablemark
This is a minor fix that incorrectly replaced the existing "elementsWithNoTextChildren" with "ELEMENTS_WITH_TEXT_CHILDREN" and changed it back to "ELEMENTS_WITH_NO_TEXT_CHILDREN". 🙏

Copy link
Owner

@remarkablemark remarkablemark left a comment

Choose a reason for hiding this comment

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

Thanks @ssi02014! LGTM

@remarkablemark remarkablemark added the bug Something isn't working label Aug 9, 2023
@remarkablemark remarkablemark merged commit b6413bd into remarkablemark:master Aug 9, 2023
@remarkablemark
Copy link
Owner

Release:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants