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

Upgrade prefixer, don't handle multiple rules at once #71

Merged
merged 4 commits into from
May 10, 2016

Conversation

zgotsch
Copy link
Contributor

@zgotsch zgotsch commented May 3, 2016

Use the new inline-style-prefix-all, which gives us a nice array of rules which need to be prefixed. Only importantify one rule at a time, so we don't have to parse css in order to pick the right semicolons :)

fixes #70

@zgotsch
Copy link
Contributor Author

zgotsch commented May 3, 2016

I probably need to sign the CLA now, will do that in a bit.

const rules = objectToPairs(prefixedDeclarations).map(([key, value]) => {
const prefixedRules = flatten(
objectToPairs(prefixedDeclarations).map(([key, value]) => {
if (typeof value === 'object') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use Array.isArray?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I thought of this but I don't know what your polyfilling setup is. If it's safe, it's way better.

@xymostech
Copy link
Contributor

This looks awesome!!

Could you add a test to see how this interacts with our stringHandlers stuff? For instance, our fontFamily string handler also accepts arrays as a valid value, and I want to make sure that this doesn't clobber that (we unfortunately don't have tests for stringHandlers yet, maybe that would answer my question).

I guess something that looks like

assertCSS('.foo', [{
    test: ['a', 'b'],
}], ".foo { test: a b !important; }", { test: (val) => val.join(" ") });

(I'm worried it would turn into ".foo { test: a !important; test: b !important; }", which isn't what we want)

@zgotsch
Copy link
Contributor Author

zgotsch commented May 3, 2016

If I understand correctly, there is already a test for this:

    it('supports custom string handlers', () => {
        assertCSS('.foo', [{
            fontFamily: ["Helvetica", "sans-serif"]
        }], '.foo{font-family:Helvetica, sans-serif !important;}', {
            fontFamily: (val) => val.join(", "),
        });
    });

which passes

@xymostech
Copy link
Contributor

Oh yep, you're right! Sweet. Did you sign the CLA? (we should really hook our CLA bot up to this repo...)

@zgotsch
Copy link
Contributor Author

zgotsch commented May 3, 2016

I did sign the CLA!

@jlfwong
Copy link
Collaborator

jlfwong commented May 4, 2016

I'm confused -- didn't we only need importantify in the first place because we weren't getting an array of values? Why do we still need the regex? Just to avoid double important'ing things?

@zgotsch
Copy link
Contributor Author

zgotsch commented May 4, 2016

Yeah, that's why I kept using it. There should probably be a test for that
so it doesn't get broken in future.
On Wed, May 4, 2016 at 4:37 AM Jamie Wong notifications@github.com wrote:

I'm confused -- didn't we only need importantify in the first place
because we weren't getting an array of values? Why do we still need the
regex? Just to avoid double important'ing things?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#71 (comment)

@zgotsch
Copy link
Contributor Author

zgotsch commented May 7, 2016

Oof, seems like prefixes are not ordered consistently across platforms. I'm going to sort them...

@zgotsch
Copy link
Contributor Author

zgotsch commented May 7, 2016

Is this safe? In the case of something like -webkit-box and -webkit-flex, the older prefix should probably go first. It's not trivial to know the rules for ordering, though. This should also probably be the responsibility of inline-prefix-all, not aphrodite.

@kentcdodds
Copy link
Contributor

This should also probably be the responsibility of inline-prefix-all, not aphrodite.

Yes

@xymostech
Copy link
Contributor

@zgotsch Okay, I'm gonna merge this!

@xymostech xymostech merged commit 78cbfd4 into Khan:master May 10, 2016
jlfwong added a commit that referenced this pull request Aug 15, 2016
I also changed the code to no longer sort the values when inline-style-prefixer
generates multiple values. It's not clear to me why exactly we were doing this,
but inline-style-prefixer seems to do the right thing. See #71 for original
context.

Fixes #100
jlfwong added a commit that referenced this pull request Aug 15, 2016
I also changed the code to no longer sort the values when inline-style-prefixer
generates multiple values. It's not clear to me why exactly we were doing this,
but inline-style-prefixer seems to do the right thing. See #71 for original
context.

Fixes #100
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.

CSS rules with semicolons break due to importantify.
4 participants