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(postcss-merge-longhand): allow mixing custom properties with regular values when merging #956

Conversation

Maistho
Copy link

@Maistho Maistho commented Sep 24, 2020

This resolves an issue where merging properties would break the css

Example of the issue

/* This declaration should not be changed */
h1{border:var(--v1) solid var(--v2, #abc123);border-right-color:blue}

/* Before this commit it would be, into this */
h1{border-right-color:var(--v2,#abc123);border-right:var(--v1) solid #00f;border-style:solid;border-width:var(--v1)}

…lar values when merging

This resolves an issue where merging properties would break the css

Example of the issue
```css
/* This declaration should not be changed */
h1{border:var(--v1) solid var(--v2, #abc123);border-right-color:blue}

/* Before this commit it would be, into this */
h1{border-right-color:var(--v2,#abc123);border-right:var(--v1) solid #00f;border-style:solid;border-width:var(--v1)}
```
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

@anikethsaha
Copy link
Member

anikethsaha commented Sep 25, 2020

how about keeping the merge check and remove the whole else block alone here ?

cause this would merge non custom props val with custom props val

@Maistho
Copy link
Author

Maistho commented Sep 27, 2020

cause this would merge non custom props val with custom props val

When would this cause an issue? I would like to add a test case for this if it does cause any issues, since the current tests all pass with my changes.

I admit to being a bit sceptical when removing the check myself, but since it didn't seem to cause any issues and it completely solved my issue it seemed fine.

@anikethsaha
Copy link
Member

Maybe, try using CSS where there is a customs value and non-custom value

It should not merge them.

@ludofischer
Copy link
Collaborator

#1057 solved this. I've added the test case to #1059

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.

None yet

4 participants