Refactor canMerge in postcss-merge-longhand #634
Conversation
jordrake
force-pushed
the
refactor-can-merge
branch
from
1e46363
to
4685cff
Compare
|
Actually this logic is not parity with the old one and will correct. Ignore for now! |
| fixture: 'h1{box-bottom:10px;box-top:initial;box-left:20px}', | ||
| expected: 'h1{box-bottom:10px;box-top:initial;box-left:20px}', | ||
| fixture: 'h1{box-bottom:10px;box-top:initial;box-left:20px;box-right:20px}', | ||
| expected: 'h1{box-bottom:10px;box-top:initial;box-left:20px;box-right:20px}', |
There was a problem hiding this comment.
Please add new tests, don't change exist, it is bad practice 👍 This can lead to regressions in unexpected places.
There was a problem hiding this comment.
These are probably duplicates of existing tests (checking for incomplete box props) hence why they did not increase coverage in the previous PR. However I appreciate the caution and have kept them the same but renamed the message to better match the area they are touching. Hope that's OK.
EDIT: In the diff, the old tests now appear above this change.
jordrake
force-pushed
the
refactor-can-merge
branch
3 times, most recently
from
7b03593
to
8c51720
Compare
| fixture: 'h1{box-bottom:10px;box-top:initial;box-left:20px;box-right:20px}', | ||
| expected: 'h1{box-bottom:10px;box-top:initial;box-left:20px;box-right:20px}', | ||
| }, { | ||
| message: 'should not merge box props where there is a mix of reserved properties', |
There was a problem hiding this comment.
In the initial refactor attempt, this test (would have) failed.
|
/cc @jordrake Ready to merge? |
| ); | ||
|
|
||
| test( | ||
| 'should not explode border with inherit properties', |
There was a problem hiding this comment.
Bad copy and paste job!
| if (props.some(hasInherit) && !props.every(hasInherit)) { | ||
| return false; | ||
| } | ||
| function hasIncompleteReservedKeyword (props) { |
There was a problem hiding this comment.
I feel that this change damaged the readability of the function. It was definitely more clear what it did before.
There was a problem hiding this comment.
Fair enough. Happy to revert this.
|
|
||
| export default (prop, includeCustomProps = true) => { | ||
| if (includeCustomProps && isCustomProp(prop)) { | ||
| return false; | ||
| } | ||
|
|
||
| return !isInherit(prop) && !isInitial(prop); | ||
| return !isReserved(prop); |
There was a problem hiding this comment.
What makes inherit and initial "reserved"`?
There was a problem hiding this comment.
I agree it is a bit vague, this is what this is referred to in other areas of this codebase. Since it's only called once I can just inline the includes and avoid naming this.
jordrake
force-pushed
the
refactor-can-merge
branch
2 times, most recently
from
562a2bc
to
5b21145
Compare
jordrake
force-pushed
the
refactor-can-merge
branch
from
5b21145
to
3c3102a
Compare
Increased the test coverage following the previous PR. I have done a small refactor to canMerge and canExplode to avoid duplication.
Not sure about the inclusion of 'none', I think it should be there but existing tests fail when I try to add it.