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

feat: add support for custom css properties #348

Merged
merged 1 commit into from
Aug 13, 2019
Merged

Conversation

jquense
Copy link
Contributor

@jquense jquense commented Aug 9, 2019

supersedes #75

At the moment the styled support is half-baked because we've no shouldForwardProps support, but i don't want to bloat the runtime right now. This is primarily for the css prop case, where we don't need to handle whitelisting

expect(code).toContain(`css={[_default, [["${i.id}", color]]]}`);
});

it.only('should disallow when configured off', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it.only('should disallow when configured off', async () => {
it('should disallow when configured off', async () => {

).rejects.toThrow(/Could not resolve interpolation to a value/);
});

it('should disallow Styled usage when configured off', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's disabled in this test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can't use the ${props => props.color} in styled, only the css prop

// Match any valid CSS units followed by a separator such as ;, newline etc.
const rUnit = new RegExp(`^(${cssUnits.join('|')})(;|,|\n| |\\))`);

// const toValidCSSIdentifier = s => s.replace(/[^_0-9a-z]/gi, '_');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@taion
Copy link
Contributor

taion commented Aug 13, 2019

On the CSS thing, would it be silly to support something like:

<div className={css`...`} />

instead of having a custom prop?

Or something like a cssClass template that, instead of just resolving to an imported CSS module, also creates and uses a custom class name?

That would allow stuff like:

<MyComponent className={cssClass`...`} fooClassName={cssClass`...`}>

@jquense
Copy link
Contributor Author

jquense commented Aug 13, 2019

tbh i went with the css prop since that's what everyone else does...it's a good question tho, there isn't really a reason why

<div className={cn(props.className, css``)} />

wouldn't work, but it's a bit harder to detect usage, to know whether css should return the css module or a single class. cssClass would work, however since it's astroturf only you'd lose editor intellisense and syntax highlighting

@taion
Copy link
Contributor

taion commented Aug 13, 2019

Yeah, hard to say how sophisticated we need to be. The inability to integrate nicely with tooling is a bummer.

I was just looking at cases where passing in more than one class would be useful. No reason not to support a css prop, but it doesn't seem like the "best" API here.

@jquense
Copy link
Contributor Author

jquense commented Aug 13, 2019

siigh yeah, so a lot of the api choices that SC and emotion made are regrettable. It's annoying to have to follow. Lets bikeshed more tho in #320

@jquense jquense merged commit a645fd1 into master Aug 13, 2019
@jquense jquense deleted the css-prop-interpolations branch August 13, 2019 23:43
@taion taion mentioned this pull request Aug 27, 2019
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.

2 participants