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

Rule list order should be preserved after Emotion.makeDict #95

Closed
wegry opened this issue Nov 16, 2018 · 5 comments
Closed

Rule list order should be preserved after Emotion.makeDict #95

wegry opened this issue Nov 16, 2018 · 5 comments

Comments

@wegry
Copy link
Contributor

wegry commented Nov 16, 2018

On current master, the order of properties is backwards from how it ultimately ends up in CSS.

  Css.[
    background(red),
    borderBottom(px(5), solid, black),
    width(px(50)),
    height(px(50)),
    margin(px(10)),
];
margin: 10px;
height: 50px;
width: 50px;
border-bottom: 5px solid #000000;
background: #FF0000;

Presumably this is because here we reverse the rule list before passing it into the JS object creator.

I noticed this because of the interaction between properties like border and border left. Order matters in this case.

Changing the order to match existing CSS was part of #92, but was stripped out with the merge changes I'd added. I've been using my own fork because of this, but I'd like to have it patched upstream. What better than a major release?

@giraud
Copy link
Owner

giraud commented Nov 16, 2018

The reverse has been introduced here #23 with the rewrite of bs-css and is discussed in the issue.
It's about merging styles (list), but the arguments are not very strong imo so I feel it's ok to remove that reverse operation.

@baldurh
Copy link
Collaborator

baldurh commented Nov 16, 2018

This will have to be explained very well in the changelog/release notes since this changes how rules override other rules. Meaning if you have

let base = Css.([color(blue)]);
let style = Css.([color(red), ...base]);

version 8 will end up with color: blue whereas earlier versions will get color: red

@wegry
Copy link
Contributor Author

wegry commented Nov 16, 2018

The fact that the previous styling was backwards is a headache, but fixing it is essential to #86 I think.

@giraud
Copy link
Owner

giraud commented Nov 16, 2018

yes, it needs to be written in release notes, but if you don't reverse the list you get exactly what you write, it's less magic

@baldurh
Copy link
Collaborator

baldurh commented Nov 16, 2018

I agree it was completely unintuitive. Overriding rules in the reverse order makes no sense and this was mainly added so that you could override rules and use the spread operator. This will not fix #86 though since you always lose duplicate selectors using this method. I’m working on a PR for new merge methods which will definitely solve this. I’m just doing final refinements to it :)

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

No branches or pull requests

3 participants