emotion-js / emotion

👩‍🎤 CSS-in-JS library designed for high performance style composition
https://emotion.sh/
MIT License
17.47k stars 1.11k forks source link

Enable styling of AMP components by whitelisting amp attributes #1453

Closed the-architect closed 5 years ago

the-architect commented 5 years ago

The problem

AMP components rely on custom attributes for their functionality.

The "is-prop-valid" package filters out these attributes, so it's currently not possible to use Emotion or StyledComponents to style AMP components.

Example:

<amp-accordion ... > // this works fine
    <section expanded> // this one can not be styled, otherwise expand gets filtered out
        ....
    </section>
</amp-accordion>

Proposed solution

Whitelist amp component custom attributes

Andarist commented 5 years ago

Actually you can do this with emotion:

import isPropValid from '@emotion/is-prop-valid'
import styled from '@emotion/styled'

const H1 = styled('h1', {
  shouldForwardProp: prop =>
    isPropValid(prop) || /^amp-/.test(prop)
})(props => ({
  color: 'hotpink'
}))

;<H1 amp-foo="bar">Something.</H1>
the-architect commented 5 years ago

@Andarist thanks for the quick response.

I made a mistake when exlpaining the issue. I've updated the description above. It's actually directly related to DOM element attributes.

I am using the styled components library, which uses your "is-prop-valid" package. According to their source code (https://github.com/styled-components/styled-components/blob/master/packages/styled-components/src/models/StyledComponent.js#L154), there is no way to do what you suggested.

FezVrasta commented 5 years ago

I'm probably missing some context, but why should a styled-components issue belong to the Emotion issue tracker?

the-architect commented 5 years ago

@FezVrasta emotion provides the package that styled-components uses for the attribute whitelisting. see my comment above. :)

FezVrasta commented 5 years ago

Yeah but Emotion itself allows what you are asking, the problem seems some lack of support from the styled-components implementation. Right?

the-architect commented 5 years ago

@FezVrasta you're right :) @probablyup what do you think. Would this be possible by adding something like the shouldForwardProp as mentioned here into StyledComponents?

Andarist commented 5 years ago

We technically could support this within isPropValid but this really seems for me like dangerous precedence. This is not a standard HTML attribute and it would be done just to accommodate "some" HTML extension? framework? It's not a standard feature and thus IMHO it doesn't belong into a generic isPropValid library. Luckily with emotion you can customize this behavior and this seems like something that should be customized by AMP developers as it's not something that is needed by everyone else. I believe styled-components should provide means to customize this behavior too.

the-architect commented 5 years ago

@Andarist I agree.

Sadly next.js AMP does not work properly with emotion, but that's another issue.