Skip to content

Conversation

@alex-wilmer
Copy link

a) I tried to run the tests but got Unexpected token export. I feel like Jest is overkill for this utility. I've been using Mocha for my own stuff so I know how to set that up for babel. Either way, it would be preferable to have tests for this PR

b) Is my solution brittle? I'm feeling hazy about the way I'm doing this, possibly because I'm expecting the array positions to be valid CSS but different than how I'm handling it here

@alex-wilmer alex-wilmer force-pushed the feat/support-rgb-spaces branch from b4fa954 to 54de5af Compare August 1, 2016 04:06

const values = color && _colorValuesThatCouldHaveSpaces.includes(color.split("(")[0])
? [...splitValues.slice(0, 2), splitValues.slice(2, Infinity).join("")]
: splitValues
Copy link

@ghost ghost Aug 1, 2016

Choose a reason for hiding this comment

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

part of the goal here was to support "1px solid green" and "1px green solid" because currently browsers support all orderings.

What do you think about removing all spaces between ( and ) instead?

const parenRegex = /\(([^)]+)\)/;
const matches = value.match(parenRegex);
// remove all spaces within color values rgb(...), rgba(...), hsl(...), and hsla(...)
const trimmedValue = matches !== null ? value.replace(matches[0], matches[0].replace(" ", "")).trim() : value.trim();
const values = trimmedValue.split(" ");

not sure the performance of regex -- might be worth writing that without the global regex like below (though jsperf is down)

const trimmedValue = function() {
  value = value.trim();
  if (value.indexOf("(") === -1) { return value; }

  const splitOnOpen = value.split("(");
  const splitOnClose = splitOnOpen[1].split(")");
  // this line can be one of two, depends again on performance
  // I'm guessing the second one below w/ regex is the faster
  return splitOnOpen[0] + "(" + splitOnClose[0].split(" ").join("") + ")" + splitOnClose[1];
  return splitOnOpen[0] + "(" + splitOnClose[0].replace(/ /g, "") + ")" + splitOnClose[1];
}();

Copy link
Author

@alex-wilmer alex-wilmer Aug 1, 2016

Choose a reason for hiding this comment

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

I think you're right, and was the answer I was looking for. Anything between parens is really just part of the same value and can be stitched together to make the rest of the function work. I'm gonna sleep on it and update my PR. Thanks 😄

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.

1 participant