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

Add utility function to split string into component values #209

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

johannesodland
Copy link
Contributor

When parsing component values passed as a string to rangeStart, rangeEnd and inset we have previously used str.split(), either with a single whitespace or with a regex with various issues:

  • 'calc(2 * 10px) auto'.split(' ') returns ['calc(2', '*', '10px)', 'auto'], breaking apart functions
  • 'calc(2 * 10px) auto'.split(/(?<!\([^\)]*)\s(?![^\(]*\))/) returns ['calc(2 * 10px)', 'auto'] as expected, but causes an error in browsers without support for lookahead

This PR adds a utility function splitIntoComponentValues(input) that splits a string with multiple component values into an array of individual component value strings. These can then be processed either as keywords or be further parsed using CSSNumericValue.parse().

splitIntoComponentValues('calc(2 * 10px) calc(2 * 5px)');
// [('calc(2 * 10px)', 'calc(2 * 5px)']

splitIntoComponentValues('10px 10px').map(val => CSSNumbericValue.parse(val));
// [new CSSUnitValue(10, 'px'), new CSSUnitValue(10, 'px')]

@johannesodland
Copy link
Contributor Author

@bramus Rebased to account for changes from #153

Copy link
Collaborator

@bramus bramus left a comment

Choose a reason for hiding this comment

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

Overall structure looks good. A few remarks to address.

src/proxy-animation.js Outdated Show resolved Hide resolved
src/scroll-timeline-base.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
@johannesodland johannesodland force-pushed the split-component-value-list branch from 1afa09f to 8e1896c Compare February 1, 2024 06:43
@johannesodland
Copy link
Contributor Author

Overall structure looks good. A few remarks to address.

Thanks for the review @bramus 🙏

I think I've addressed all your remarks: https://github.com/flackr/scroll-timeline/compare/1afa09f2c7a3c39efc4a348f0940fd6fb6b03085..8e1896c3c547119a6bafed25ffd764f0efd786b4

@bramus
Copy link
Collaborator

bramus commented Feb 1, 2024

Just came to the realization one can perfectly author something like animation-range: calc(0% + 50px)calc(100% - 50px) – with no spaces between the start and end – which isn’t handled by splitIntoComponentValues().

Now, I don’t think anyone would write such a declaration and consider this be an extreme edge case, so I’m fine with merging this as is.

/ping @flackr for an opinion on this.

@bramus bramus requested a review from flackr February 1, 2024 08:51
@johannesodland
Copy link
Contributor Author

Just came to the realization one can perfectly author something like animation-range: calc(0% + 50px)calc(100% - 50px) – with no spaces between the start and end – which isn’t handled by splitIntoComponentValues().

We could easily handle that in splitIntoComponentValues() when we consume the end parenthesis.

@johannesodland johannesodland force-pushed the split-component-value-list branch from 8e1896c to 14cde5b Compare February 1, 2024 16:17
@johannesodland
Copy link
Contributor Author

@bramus bramus merged commit 770cb37 into flackr:master Feb 1, 2024
2 checks passed
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.

2 participants