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

fix: don't mess with values that reference custom props #64

Merged
merged 2 commits into from
Feb 4, 2020

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Jan 30, 2020

Ran into this on a gatsby site. shouldn't try and fix values that are custom properties

@luisrudge luisrudge merged commit 04e66ac into luisrudge:master Feb 4, 2020
@luisrudge
Copy link
Owner

Released in 4.2.0

@luisrudge
Copy link
Owner

@jquense thanks for the PR! 🎉

@jquense jquense deleted the custom-properties branch February 4, 2020 13:59
@Timer
Copy link

Timer commented Feb 19, 2020

Why are the values untouched?

Input:

.flex-parsing {
  flex: 0 0 calc(50% - var(--vertical-gutter));
}

Output before this PR:

.flex-parsing {
  flex-grow: 0;
  flex-shrink: 0;
  flex-basis: calc(50% - var(--vertical-gutter);
}

We seem to be missing a closing parenthesis.

Output after this PR:

.flex-parsing {
  flex: 0 0 calc(50% - var(--vertical-gutter));
}

@luisrudge
Copy link
Owner

@Timer can you send a PR with a failing test + the fix? Thanks!

@Timer
Copy link

Timer commented Feb 19, 2020

@jquense were you targeting a specific issue or was this a workaround for the closing parenthesis issue?

I can send a PR if you confirm this was a workaround and not the intended bug fix!

@jquense
Copy link
Contributor Author

jquense commented Feb 19, 2020

It targets a specific issue. This:

flex: var(--flex);

was being incorrectly converted to flex: var(--flex) 1; (note the 1)

What exactly is this breaking for you? The example you gave looks likes it's actually fixing an unrelated bug in your code by adding the missing paren? I'm not sure why it would add them tho, since it's not touching the value at all

@Timer
Copy link

Timer commented Feb 19, 2020

I believe you misread the code example, this plugin is removing the paren and causing one to be missing.

If we forget this PR for a second, this was the observed behavior:

/* input */
.flex-parsing {
  flex: 0 0 calc(50% - var(--vertical-gutter));
}

/* output */
.flex-parsing {
  flex-grow: 0;
  flex-shrink: 0;
  flex-basis: calc(50% - var(--vertical-gutter); /* <-- note it dropped a closing paren */
}

@jquense
Copy link
Contributor Author

jquense commented Feb 19, 2020

i'm either reading it wrong or it's written backwards...In your first post it looks like the "Output before this PR:" is missing the the paren and "Output after this PR:" contains it

In any case, the bug is a bit surprising since this change only prevents the plugin from touching a value, it doesn't process them at all. It ends up following the same path as if it was 'none'. We should fix it tho!

@Timer
Copy link

Timer commented Feb 19, 2020

Aah, I was saying the output after this PR "fixed" it by completely disabling the flexbug correction. But it doesn't seem like the desired behavior (to be disabled for this case).

From my understanding, your PR was meant to fix this case:

* { flex: var(--flex); }

But it accidentally disabled the transform for this case (where var is unrelated to flex attributes):

* { flex: 0 0 calc(50% - var(--vertical-gutter)); }

@stof
Copy link

stof commented Feb 25, 2020

@Timer but the "fixed" version of this does not contain any fix anyway, as it sets exactly the same values than the longhand.

@Timer
Copy link

Timer commented Feb 26, 2020

Can you elaborate on what you mean @stof?

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.

4 participants