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

Add px to unitless numbers when interpolate objects #2173

Merged
merged 15 commits into from Nov 12, 2018

Conversation

Projects
None yet
4 participants
@Fer0x
Contributor

Fer0x commented Oct 30, 2018

Fix #2165, #825

This PR highly based on slightly modified react-dom internal code.
It adds px to all number values excerpt whitelisted properties.
I know that will a slightly bloat library, but it will be very helpful for those, who is moving from object-like css-in-js solutions to styled-components.

@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 30, 2018

Ugh that adds almost half a kB

@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Oct 30, 2018

@probablyup yep, would be better if react-dom just exported this function.

@probablyup

This comment has been minimized.

Contributor

probablyup commented Nov 1, 2018

We could leverage this emotion package to save having to maintain it locally: https://github.com/emotion-js/emotion/tree/master/packages/unitless

Fer0x added some commits Nov 1, 2018

@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Nov 1, 2018

if (
typeof value === 'number' &&
value !== 0 &&

This comment has been minimized.

@probablyup

probablyup Nov 2, 2018

Contributor

This line can probably go. There's no harm to 0 vs 0px and some older browsers want the unit anyway.

This comment has been minimized.

@Fer0x

Fer0x Nov 2, 2018

Contributor

@probablyup unitless zero is part of CSS 2.1 which is supported even in IE 8.
As i know, React supports IE 9+, which makes adding px to 0 senseless.
This file is mostly copied "as it" from react source code. I think it will be strange when react itself doesn't apply px to zero, but styled-components will do it.

This comment has been minimized.

@probablyup

probablyup Nov 2, 2018

Contributor

Yes but it's not required so let's save the few bytes

This comment has been minimized.

@kitten

kitten Nov 5, 2018

Member

I don't want to be nitpicky, but a few bytes that we save here is putting more strain on gzip; either way were talking 6-7 chars after uglification. The bulk is still the map of properties 😅

return value;
}
const isEmpty = value == null || typeof value === 'boolean' || value === '';

This comment has been minimized.

@probablyup

probablyup Nov 2, 2018

Contributor

let's remove the variable assignment since it isn't being used again

export default function dangerousStyleValue(name: string, value: any): any {
// https://github.com/amilajack/eslint-plugin-flowtype-errors/issues/133
// $FlowFixMe
if (typeof value === 'symbol') {

This comment has been minimized.

@probablyup

probablyup Nov 2, 2018

Contributor

I can't think of when this would ever happen, let's remove.

This comment has been minimized.

@Fer0x

Fer0x Nov 2, 2018

Contributor

@probablyup it necessary for this:

    const Bar = styled.div`
      ${Foo}: {
        background-color: red;
      };
    `;

This comment has been minimized.

@probablyup

probablyup Nov 2, 2018

Contributor

How so? Nothing in that block is a symbol.

This comment has been minimized.

@Fer0x

Fer0x Nov 2, 2018

Contributor

@probablyup because flatten only warns about passing non styled component but doesn't break function execution. So, here https://github.com/styled-components/styled-components/blob/master/src/utils/flatten.js#L88 React component passed to objToCss where it loops over all React component properties.

This comment has been minimized.

@probablyup

probablyup Nov 2, 2018

Contributor

Ah, let's do an isValidElement check to just skip processing that stuff then.

This comment has been minimized.

@Fer0x

This comment has been minimized.

@probablyup

probablyup Nov 4, 2018

Contributor

Or even throw an actual error...

This comment has been minimized.

@Fer0x

Fer0x Nov 12, 2018

Contributor

@probablyup

How about to throw StyledError here in both dev/prod envs?

      if (process.env.NODE_ENV !== 'production') {
        /* Warn if not referring styled component */
        try {
          // eslint-disable-next-line new-cap
          if (isElement(new chunk(executionContext))) {
            console.warn(
              `${getComponentName(
                chunk
              )} is not a styled component and cannot be referred to via component selector. See https://www.styled-components.com/docs/advanced#referring-to-other-components for more details.`
            );
          }
          // eslint-disable-next-line no-empty
        } catch (e) {}
      }

This comment has been minimized.

@probablyup

probablyup Nov 12, 2018

Contributor

Yeah, let's do it.

if (
typeof value === 'number' &&
value !== 0 &&
!(unitless.hasOwnProperty(name) && unitless[name] === 1)

This comment has been minimized.

@probablyup

probablyup Nov 2, 2018

Contributor

I think this can just be simplified to !(name in unitless)

import unitless from '@emotion/unitless';
// Taken from https://github.com/facebook/react/blob/b87aabdfe1b7461e7331abb3601d9e6bb27544bc/packages/react-dom/src/shared/dangerousStyleValue.js
export default function dangerousStyleValue(name: string, value: any): any {

This comment has been minimized.

@probablyup

probablyup Nov 2, 2018

Contributor

This naming is a little ominous, let's change it to addUnitIfNeeded

@probablyup

This comment has been minimized.

Contributor

probablyup commented Nov 2, 2018

Changelog entry too afterward 😄

@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Nov 12, 2018

@probablyup ready for final review

@Fer0x Fer0x requested a review from probablyup Nov 12, 2018

@probablyup probablyup merged commit 3af9bef into master Nov 12, 2018

3 checks passed

bundlesize ./dist/styled-components.min.js: 15.61KB < maxSize 16KB (gzip)(284B larger than master, careful!)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@probablyup probablyup deleted the add-px-to-unitless branch Nov 12, 2018

@dsabanin

This comment has been minimized.

dsabanin commented on src/utils/flatten.js in 9a2ad9d Nov 12, 2018

After upgrading from 4.0.3 to 4.1.0 this line started throwing 'chunk is not a constructor', had to rollback and everything was working again.

This comment has been minimized.

Contributor

probablyup replied Nov 12, 2018

4.1.1 should fix it, sorry

This comment has been minimized.

Contributor

probablyup replied Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment