atellmer / dark

The lightweight and powerful UI rendering engine without dependencies and written in TypeScript💫 (Browser, Node.js, Android, iOS, Windows, Linux, macOS)
MIT License
45 stars 2 forks source link

styled bad order #72

Open einar-hjortdal opened 6 months ago

einar-hjortdal commented 6 months ago
const LinksRow = styled.div`
  display: flex;
  justify-content: space-between;
  gap: 2.5vw;
  & ul {
    list-style-type: none;
    padding: 0;
    margin: 0;
    width: 100%;
  }
  & ul li {
    list-style: none;
    border-bottom: ${props => props.theme.borderStyle};
  }
  & ul li a {
    display: flex;
    justify-content: space-between;
    align-items: center;
    text-decoration: none;
    color: ${props => props.theme.black};
    padding: .5vw 0;
    transition: color .2s, background-color .2s, padding .3s;
  }
  & ul li a:hover {
    color: ${props => props.theme.white};
    background-color: ${props => props.theme.black};
    padding: .5vw;
  }
  @media (prefers-reduced-motion: reduce) {
    & ul li a {
      transition: none;
    }
  }
`

the media query style is outputted before the & ul li a style.

@media (prefers-reduced-motion: reduce) {
  .dk-cadhch {}

  .dk-cadhch ul li a {
    transition: none;
  }
}

.dk-bfhafa {}

.dk-bfhafa ul li {
  list-style: none;
  border-bottom: solid 1px #1e1f1f;
}

.dk-bdfidj {}

.dk-bdfidj ul li a {
  display: flex;
  justify-content: space-between;
  align-items: center;
  text-decoration: none;
  color: #1e1f1f;
  padding: .5vw 0;
  transition: color .2s, background-color .2s, padding .3s;
}

Because they have the same specificity, the browser ignores the media query styles. I believe the media query should be put after the styles defined before it.

atellmer commented 6 months ago

As far as I remember, media-rules are added to the end, but depending on whether the style is static or dynamic. The order is: static style + static media + dynamic style + dynamic media.

Try workaround:

@media (prefers-reduced-motion: reduce) {
    & ul li a {
      transition: ${() => 'none'}; // fn
    }
 }
einar-hjortdal commented 6 months ago

Try workaround:

@media (prefers-reduced-motion: reduce) {
    & ul li a {
      transition: ${() => 'none'}; // fn
    }
 }

The order is unchanged. Besides, I do not think it is a good solution: I really have to think about the way styled works to sort my attributes based on that. I think I defined the media query at the end and it should be rendered at the end, according to the order I defined it.

atellmer commented 6 months ago

Let's say we have this code and media-query is inserted at the end when mounting the component. Everything will work. But what happens when a new component with a different color appears? The style will be calculated and inserted at the end of the style tag and it will no longer be correct. What to do in this case, provided that we cannot touch already inserted styles. Otherwise, we would have to parse the already inserted style blocks from the entire application every time: something has changed and we parse the entire style tag again, rearrange the styles and rewrite the tag again. This looks like a second parsing job and it will most likely be slow. What to do in this case?

const Box = styled.div`
  & a {
    color: ${props => props.$color};
  }

  @media (max-width: 800px) {
    & a {
      color: red;
    }
  }
`;

Maybe it is good case for !important or some more specific selector?

const Box = styled.div`
  & a {
    color: ${props => props.$color};
  }

  @media (max-width: 800px) {
    & a[href] {
      color: red;
    }
  }
`;
einar-hjortdal commented 6 months ago

Shouldn't styled create a new class for the different color and insert it afterwards?

const Box = styled.div`
  & a {
    color: ${props => props.$color};
  }

  @media (max-width: 800px) {
    & a {
      color: red;
    }
  }
`;
.class-one a {
  color: blue;
}
@media (max-width: 800px) {
  .class-one a {
    color: red;
  }
}

.class-two a {
  color: green;
}
@media (max-width: 800px) {
  .class-two a {
    color: red;
  }
}
atellmer commented 6 months ago

Then we need to support one more nesting, because & in your example refers to different class names: the first refers to a dynamic class, and the second to a static one.

I don't know how easy it is to add this to the current architecture. We need to see if this can be done in the original styled-components.

const Box = styled.div`
  & a {
    color: ${props => props.$color};

    @media (max-width: 800px) {
       color: red;
    }
  }
`;
einar-hjortdal commented 6 months ago

Outdated react and styled component versions because I just cloned the first random github repository to test this

const Box = styled.div`
  & a {
    font-weight: bold;
    color: ${props => `${props.$color}`};
  }

  @media (max-width: 800px) {
    & a {
      color: red;
    }
  }
`;

class App extends Component {
  render() {
    return (
      <>
      <Box $color='blue'>
        <a>blue</a>
      </Box>
      <Box $color='green'>
        <a>green</a>
      </Box>
      </>
    );
  }
}
<style data-styled="active" data-styled-version="5.3.3">
.gLrqxF a{font-weight:bold;color:blue;}@media (max-width:800px){.gLrqxF a{color:red;}}
.eKiNWE a{font-weight:bold;color:green;}@media (max-width:800px){.eKiNWE a{color:red;}}
</style>

I think this is the behavior I expected


This also seems to work, this syntax looks more like LESS (what I am used to) but the previous one is more similar to standard CSS, which I think is most important

const Box = styled.div`
  & a {
    font-weight: bold;
    color: ${props => `${props.$color}`};
    @media (max-width: 800px) {
      color: red;
  }
`;
<style data-styled="active" data-styled-version="5.3.3">
.kscQJB a{font-weight:bold;color:blue;}@media (max-width:800px){.kscQJB a{color:red;}}
.kSMeAQ a{font-weight:bold;color:green;}@media (max-width:800px){.kSMeAQ a{color:red;}}
</style>
atellmer commented 6 months ago

If we change the code a little, we will see this in styled-components

const Box = styled.div`
  background-color: blue; // added
  color: black; // added

  & a {
    font-weight: bold;
    color: ${(props) => `${props.$color}`};
  }

  @media (max-width: 800px) {
    & a {
      color: red;
    }
  }
`;
.iLKKBR {
  background-color: blue;
  color: black;
}

.iLKKBR a {
  font-weight: bold;
  color: blue;
}

@media (max-width: 800px) {
  .iLKKBR a {
    color: red;
  }
}

.duYaUQ {
  background-color: blue;
  color: black;
}

.duYaUQ a {
  font-weight: bold;
  color: green;
}

@media (max-width: 800px) {
  .duYaUQ a {
    color: red;
  }
}

It looks like it just stupidly duplicates all the styles for the new props. I had a better opinion of them. This just generates a bunch of duplicate code. And the css of the component can be quite large, apparently it will repeat all this.

Dark generates this (without empty classes, i will fix it later)

.dk-caccjf {
  background-color: blue;
  color: black;
}

@media (max-width: 800px) {
  .dk-caccjf a {
    color: red !important; // for now
  }
}

.dk-dbegje a {
  font-weight: bold;
  color: blue;
}

.dk-iaijad a {
  font-weight: bold;
  color: green;
}

Anyway, I will think about it.

einar-hjortdal commented 6 months ago

It looks like it just stupidly duplicates all the styles for the new props. I had a better opinion of them. This just generates a bunch of duplicate code. And the css of the component can be quite large, apparently it will repeat all this.

You're right, in styled-components 5.3.3

const Box = styled.div`
  background-color: lightgray;
  color: black;
  & a {
    font-weight: bold;
    color: ${props => `${props.$color}`};
    @media (max-width: 800px) {
      color: red;
  }
`;

class App extends Component {
  render() {
    return (
      <>
      <Box $color='blue'>
        <a>blue</a>
      </Box>
      <Box $color='green'>
        <a>green</a>
      </Box>
      </>
    );
  }
}

results in

.biclYi {
  background-color: lightgray;
  color: black;
}

.biclYi a {
  font-weight: bold;
  color: blue;
}

@media (max-width:800px) {
  .biclYi a {
    color: red;
  }
}

.bLMHlz {
  background-color: lightgray;
  color: black;
}

.bLMHlz a {
  font-weight: bold;
  color: green;
}

@media (max-width:800px) {
  .bLMHlz a {
    color: red;
  }
}

bLMHlz is a duplicate of biclYi. That is wasteful, although I believe it may not be as bad when utilizing gzip. React is also quite a large piece of code compared to Dark. On what version did you test this?

atellmer commented 6 months ago

On what version did you test this?

"dependencies": {
   "react": "18.3.1",
   "react-dom": "18.3.1",
   "styled-components": "6.1.11"
},

gzip can certainly help, but we stand for beautiful solutions, and repeating styles that do not require repetition is not beautiful.

einar-hjortdal commented 6 months ago
color: red !important; // for now

I've been trained to be afraid of !important, I don't have much experience with that. Wouldn't it cause problem with multiple media queries?


const Box = styled.div`
& a {
font-weight: bold;
color: ${props => `${props.$color}`};
}

@media (min-width: 500px) { & a { color: red; }

@media (min-width: 900px) { & a { color: teal; } } `;



> gzip can certainly help, but we stand for beautiful solutions, and repeating styles that do not require repetition is not beautiful.

I absolutely agree, as long as it works.
Update: I thought about this a bit more and I do think it is also important for the API to not be complex. I personally want to just write something as close as the CSS I would have written in a CSS module when using styled, any extra complexity is going to make me want to use another solution. There already is enough complexity, too much in this field, to add more.
atellmer commented 6 months ago

!important can be useful, but in this case you can use some special attribute that can be attached to the tag. For example, data-spec or some other name. This is again like a workaround.

@media (min-width: 500px) {
  & a[data-spec] {
    color: red;
  }
}

In this case, these expressions will be added in the order they are written, so here you set the priority yourself.

@media (min-width: 500px) {
  & a {
    color: red;
  }
}

@media (min-width: 900px) {
  & a {
    color: teal;
  }
}
atellmer commented 6 months ago

Update: I thought about this a bit more and I do think it is also important for the API to not be complex. I personally want to just write something as close as the CSS I would have written in a CSS module when using styled, any extra complexity is going to make me want to use another solution. There already is enough complexity, too much in this field, to add more.

Based on this logic, in your case it would be ideal to use CSS variables rather than generate styles dynamically. This would remove this problem and work faster because there is no js calculations. At the same time, the theme could also be switched, simply changing the variables in <GlobalStyle />, for example --black, --borderStyle and others.

einar-hjortdal commented 6 months ago

Based on this logic, in your case it would be ideal to use CSS variables rather than generate styles dynamically. This would remove this problem and work faster because there is no js calculations. At the same time, the theme could also be switched, simply changing the variables in <GlobalStyle />, for example --black, --borderStyle and others.

I do that already, but the original issue was about the wrong position of media queries generated with

  @media (prefers-reduced-motion: reduce) {
    & ul li a {
      transition: none;
    }
  }

In this situation I guess !important can work, but I wouldn't have reached for an !important here.

Again, I do not want to think in which situation I should use one technique and in which situation I should use another. Maintaining such a codebase would be awful: some media queries here some there, some use weird selectors some don't. If the solution does not generate optimal code but it leads to less headaches, it may just be the better solution to the problem. Maybe in the future a better solution can be found.

atellmer commented 6 months ago

These solutions have been proposed as hacks until I can make fixes to the codebase. But I also have to say that I won't be able to contribute them as often as I'd like, because open-source is not where I work.

einar-hjortdal commented 6 months ago

These solutions have been proposed as hacks until I can make fixes to the codebase. But I also have to say that I won't be able to contribute them as often as I'd like, because open-source is not where I work.

Of course. I think there was a misunderstanding, I had the impression that this issue was not being acknowledged as an issue. I apologize if I came across as demanding.

atellmer commented 6 months ago

In the new version this code will generate these styles

const Box = styled.div`
  background-color: pink;
  color: black;

  & a {
    font-weight: bold;
    color: ${p => `${p.$color}`};

    @media (max-width: 800px) {
      color: red;
    }
  }
`;

createRoot(document.getElementById('root')).render(
  <>
    <Box $color='blue'>
      <a>blue</a>
    </Box>
    <Box $color='green'>
      <a>green</a>
    </Box>
  </>,
);
.dk-bjjdjh {
  background-color: pink;
  color: black;
}

.dk-ggedee a {
  font-weight: bold;
  color: blue;
}

@media (max-width: 800px) {
  .dk-ggedee a {
    color: red;
  }
}

.dk-bjajid a {
  font-weight: bold;
  color: green;
}

@media (max-width: 800px) {
  .dk-bjajid a {
    color: red;
  }
}
atellmer commented 6 months ago

I just realized that if we had written it like this, we wouldn’t have had any problems in the first place.

const Box = styled.div`
  background-color: pink;
  color: black;

  ${p => css`
    & a {
      font-weight: bold;
      color: ${p.$color};
    }

    @media (max-width: 800px) {
      & a {
        color: red;
      }
    }
  `}
`;

This example brought me to this idea. It will break everything again, so nested expressions are not a solution

const Box = styled.div`
  background-color: pink;
  color: ${p => p.$color};

  @media (max-width: 800px) {
    color: red;
  }
`;

In this case, it would be correct to write like this

const Box = styled.div`
  background-color: pink;

  ${p => css`
    color: ${p.$color};

    @media (max-width: 800px) {
      color: red;
    }
  `}
`;

Sorry that I did not immediately come to this decision and suggested options with !important and others. This is the only correct approach given the architecture based on avoiding duplicates of CSS.

Your example

const LinksRow = styled.div`
  display: flex;
  justify-content: space-between;
  gap: 2.5vw;
  & ul {
    list-style-type: none;
    padding: 0;
    margin: 0;
    width: 100%;
  }
  & ul li {
    list-style: none;
    border-bottom: ${props => props.theme.borderStyle};
  }
  ${props => css`
    & ul li a {
      display: flex;
      justify-content: space-between;
      align-items: center;
      text-decoration: none;
      color: ${props.theme.black};
      padding: 0.5vw 0;
      transition: color 0.2s, background-color 0.2s, padding 0.3s;
    }
    & ul li a:hover {
      color: ${props.theme.white};
      background-color: ${props.theme.black};
      padding: 0.5vw;
    }
    @media (prefers-reduced-motion: reduce) {
      & ul li a {
        transition: none;
      }
    }
  `}
`;

I understand that when writing you have to think about how to do it. But it seems to me that this is a rather specific case, when we write styles not only for the root tag itself, but also for nested ones, and at the same time we want to interrupt the specificity of selectors in the media expression.

einar-hjortdal commented 6 months ago

I am trying to understand how I should be thinking to obtain the expected behavior in the future: I should wrap media queries (even those that don't use props) into a function, together with the attributes that they change? In the situation above, for example, the & ul li a selector appears in the media query, therefore it should be wrapped in the function with the media query? :thinking:

atellmer commented 6 months ago

To be honest, I’m still thinking about some kind of single rule. I also came to this variant - writing the problematic property separately so that it does not end up in the dynamic class if it does not depend on props. If it has such a dependence, then write all its uses inside the function.

const LinksRow = styled.div`
  display: flex;
  justify-content: space-between;
  gap: 2.5vw;
  & ul {
    list-style-type: none;
    padding: 0;
    margin: 0;
    width: 100%;
  }
  & ul li {
    list-style: none;
    border-bottom: ${props => props.theme.borderStyle};
  }
  & ul li a {
    display: flex;
    justify-content: space-between;
    align-items: center;
    text-decoration: none;
    color: ${props => props.theme.black};
    padding: 0.5vw 0;
  }

  & ul li a:hover {
    color: ${props => props.theme.white};
    background-color: ${props => props.theme.black};
    padding: 0.5vw;
  }

  & ul li a {
    transition: color 0.2s, background-color 0.2s, padding 0.3s; // write this prop separately
  }

  @media (prefers-reduced-motion: reduce) {
    & ul li a {
      transition: none;
    }
  }
`;

Or we should regenerate the entire CSS of the component, as is done in styled-components.

einar-hjortdal commented 6 months ago

I am not sure how this is related, but I am fairly sure it is related somehow. Lately I am being haunted by problems caused by styled inserting styles in unpredictable orders, specifically media queries. I haven't found an explanation for why this is happening: depending on which page is the first requested, media queries for the next-requested pages are generated before base styles, breaking media query functionality.

For example: This is the style injected when requesting the page /contact on first request image

And this is the style injected when navigating to the page /contact after a first request to another page image

const HeroBottom = styled.div`
  height: 40%;
  padding: 0 2.5vw;
  display: flex;
  flex-direction: column;
  ${props => css`
    @media (min-width: ${props.theme.sm}) {
      flex-direction: row;
    }
  `}
`

Update: I think I have found the issue. There is another component on another page (let's say page B) that also has the following code, it seems like styled generates a class and injects it in the header when page B is requested. Then, when navigating to /contact, styled injects the styles of the new page into the header. The class is not injected again, and it uses the previously-injected name, causing an order conflict.

  ${props => css`
    @media (min-width: ${props.theme.sm}) {
      flex-direction: row;
    }
  `}
.dk-fbhbcj {}

@media (min-width: 576px) {
  .dk-fbhbcj {
    flex-direction: row;
  }
}

So I guess the issue is indeed related.

Another update: A similar issue appears in other occasions, where media queries are not involved, whenever styled figures out it can reuse classes. I have just encountered a similar issue on the same page of first request where 2 independent components utilize

  ${props => {
    if (props.$isVisible === true) {
      return css`
        transform: translateY(0);
      `
    }
  }}

in this situation, the second component's base styles overrides transform: translateY(0) because it appears later in the <style> tag. This problem is not encountered when using a ternary operator instead transform: ${props => props.$isVisible ? 'translateY(0)' : 'translateY(-100%)'} but I believe both should be valid

atellmer commented 5 months ago

We should probably abandon the approach of reusing styles if it causes such problems. I'll think about it.