Skip to content

Conversation

cecton
Copy link
Contributor

@cecton cecton commented Oct 2, 2021

As mention by @AethanFoot #472 (comment) I accidentally broke the parsing behavior of Margins.

Existing behavior before my change:

  • no values: gives a warning, assume 10px on all side
  • 1 value: assume that value on all side (match CSS)
  • 2 values: assume the first value for top and right and the second value for bottom and left
    ⚠️ This is contradicting CSS where 2 values means: first value for top and bottom and second value for right and left
  • 3 values: assume the first value for top, second value for right, third value for bottom and left
    ⚠️ This is contradicting CSS where 3 values means: first value for top, second value for right and left, third value for bottom
  • 4 values: top, right, bottom, left (match CSS)
  • more than 4 values: gives a warning, assume 4 values

What I implemented:

  • no values: assume 0 on all side, no warning ❌
  • 1 value: assume that value on all side (match CSS) ✔️
  • 2 to 3 values: assign value to top, right, bottom and left, use 0 if one is missing ❌
  • 4 values: top, right, bottom, left (match CSS) ✔️
  • more than 4 values: does not parse (fatal) ❌

❌ means different from original behavior
✔️ means identical to original behavior

This PR restore the original behavior but I think we should modify it to comply with CSS so it will feel more intuitive. And I think 0 or more than 4 values should be parsing errors (fatal) so it will shows in leftwm-check.

Let me know your opinion.

@AethanFoot AethanFoot mentioned this pull request Oct 2, 2021
@mautamu
Copy link
Member

mautamu commented Oct 2, 2021

I believe CSS-like was the original goal when it was implemented; would probably be good to do so now.

@cecton
Copy link
Contributor Author

cecton commented Oct 2, 2021

I believe CSS-like was the original goal when it was implemented; would probably be good to do so now.

Ok so I adapted the 2 and 3 input values to match CSS better.

I also changed the 0 or more than 4 values to fail instead of being permissive. If you don't like this change, feel free to revert the last commit.

@lex148 lex148 merged commit 884080b into leftwm:master Oct 4, 2021
@cecton cecton deleted the fix-margin-parser branch October 4, 2021 13:49
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.

3 participants