Skip to content

add support for custom styles beginning with "--*"#82

Closed
samikarak wants to merge 0 commit intoremarkablemark:masterfrom
samikarak:master
Closed

add support for custom styles beginning with "--*"#82
samikarak wants to merge 0 commit intoremarkablemark:masterfrom
samikarak:master

Conversation

@samikarak
Copy link
Copy Markdown
Contributor

see #81

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 13, 2018

Coverage Status

Coverage decreased (-0.01%) to 99.281% when pulling 58dd941 on samikarak:master into 9eeef55 on remarkablemark:master.

Copy link
Copy Markdown
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 for opening the PR @samikarak. Do you mind resolving the comments and adding tests?

lib/utilities.js Outdated

// no hyphen found
if (string.indexOf('-') === -1) {
// customStyle or no hyphen found
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

chore: s/customStyle/custom properties?

lib/utilities.js Outdated
// no hyphen found
if (string.indexOf('-') === -1) {
// customStyle or no hyphen found
if (string.startsWith('--') || string.indexOf('-') === -1) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

fix: do you mind testing the match using regex since startsWith is not supported in IE?

// regex that matches custom property or no hyphen
var CUSTOM_PROPERTY_OR_NO_HYPHEN_REGEX = /^--[a-zA-Z]+$|^[^-]+$/;

function camelCase(string) {
  // ...

  if (CUSTOM_PROPERTY_OR_NO_HYPHEN_REGEX.test(string)) {
    return string;
  }

  // ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

seems to not work with custom styles having lisp-case/dash-case in between ...

for example:

CUSTOM_PROPERTY_OR_NO_HYPHEN_REGEX.test('customStyle') // returns : true
CUSTOM_PROPERTY_OR_NO_HYPHEN_REGEX.test('--customStyle') // returns : true
CUSTOM_PROPERTY_OR_NO_HYPHEN_REGEX.test('--custom-style') // returns : false

I think it should also be possible to use custom style having dashes in between (e.g. '--custom-style'). Therefore lets consider an easier approach:

// customStyle or no hyphen found
  if (string.indexOf('--') === 0 || string.indexOf('-') === -1) {
    return string;
  }

It could be possible to use a regex for this check, or to directly replace the string by camelCase if needed, but since javascript does not fully support lookbehind-assertions, the regex gets a little tricky ...

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Good point about the lisp-case. What about the modified regex?

-var CUSTOM_PROPERTY_OR_NO_HYPHEN_REGEX = /^--[a-zA-Z]+$|^[^-]+$/;
+var CUSTOM_PROPERTY_OR_NO_HYPHEN_REGEX = /^--[a-zA-Z-]+$|^[^-]+$/;

The indexOf also works, but do you think it's worth making the declaration value check strict?

@remarkablemark remarkablemark added the feature New feature or request label Dec 14, 2018
@remarkablemark
Copy link
Copy Markdown
Owner

Just a heads up, I pushed some commits to master so feel free to rebase your branch:

$ git pull --rebase https://github.com/remarkablemark/html-react-parser.git master
$ git push origin master --force

@remarkablemark
Copy link
Copy Markdown
Owner

Also, do you mind updating your commit messages to use the conventional commit format?

feat(utilities): add support for custom styles beginning with "--*"

It uses the commit type to help with releasing (otherwise, the version bump wouldn't be correct).

@remarkablemark
Copy link
Copy Markdown
Owner

@samikarak Did you accidentally close this PR?

@samikarak
Copy link
Copy Markdown
Contributor Author

I had to rebase my fork (I had some issues with some workspace files) and requested another PR. This PR got closed automatically. Lets continue on the new PR

@remarkablemark
Copy link
Copy Markdown
Owner

Sounds good, thanks for the update @samikarak

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants