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

chore(www): migrate to emotion 10 #11771

Merged
merged 5 commits into from
Feb 28, 2019
Merged

Conversation

DSchau
Copy link
Contributor

@DSchau DSchau commented Feb 14, 2019

Description

Migrate www/ to emotion 10

@DSchau DSchau requested a review from a team as a code owner February 14, 2019 16:36
@DSchau
Copy link
Contributor Author

DSchau commented Feb 14, 2019

Marking this a WIP - I tested it for all of 5 minutes 🤷‍♂️

Will revisit this later today.

@t2ca
Copy link
Contributor

t2ca commented Feb 14, 2019

As discussed previously here:

#10291 (comment)

The code below will break:

www/src/components/navigation-mobile.js

const getNavItemStyles = ({ isPartiallyCurrent }) =>
  isPartiallyCurrent
    ? {
        className: css({
          ...styles.link.default,
          ...styles.link.active,
          ...styles.svg.active,
        }),
      }
    : {
        className: css({
          ...styles.link.default,
          ...styles.svg.default,
        }),
      }

In my project, i just used the activeStyle prop in the Link component below:

const MobileNavItem = ({ linkTo, label, icon }) => (
  <Link to={linkTo} getProps={getNavItemStyles}>
    <span dangerouslySetInnerHTML={{ __html: icon }} />
    <div>{label}</div>
  </Link>
)

@t2ca
Copy link
Contributor

t2ca commented Feb 14, 2019

Couldn't we do something like this

const MobileNavItem = ({ linkTo, label, icon }) => (
  <Link
    to={linkTo}
    css={{ ...styles.link.default, ...styles.svg.default }}
    activeStyle={{ ...styles.link.active, ...styles.svg.active }}
  >
    <span dangerouslySetInnerHTML={{ __html: icon }} />
    <div>{label}</div>
  </Link>
)

@DSchau
Copy link
Contributor Author

DSchau commented Feb 14, 2019

@t2ca no - unfortunately. We use custom syntax (e.g. && for specificity) and other things.

I'm thinking of just adding an attribute that we can style against. See 7c6a0cc

@DSchau DSchau added status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response and removed status: WIP labels Feb 26, 2019
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

no breaking things, just adding a few comments. I'll approve anyway

@@ -178,7 +178,7 @@ class Form extends React.Component {
type="email"
required
autoComplete="email"
innerRef={input => {
ref={input => {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we move to createRef?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the benefit? Shouldn't these work exactly the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

no benefit just following best practices

The examples below have been updated to use the React.createRef() API introduced in React 16.3. If you are using an earlier release of React, we recommend using callback refs instead.
https://reactjs.org/docs/refs-and-the-dom.html

it was more like oh he changes innerRef to ref, maybe update using createRef too but no biggy 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! I've always thought that createRef was providing an alternate/better mechanism to string refs, e.g. ref="someValue". I didn't know if it's preferred over callback refs, but rather just seems to be an alternate API?

Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment there is no real benefit for it but I can imagine that they will deprecate old behaviour in the next release as with createRef the framework has a bit more control

}),
}
const getProps = ({ isPartiallyCurrent }) =>
isPartiallyCurrent && { "data-active": true }
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not a fan of this syntax though 😋 (just preference)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you change it? (I don't like it that much either!)

Copy link
Contributor

@wardpeet wardpeet Feb 27, 2019

Choose a reason for hiding this comment

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

const getProps = ({ isPartiallyCurrent }) => {
  if (!isPartiallyCurrent ) {
    return
  }
  return { "data-active": true }
}

it's more code but the minifier takes care of it.

Copy link
Contributor

@Kosai106 Kosai106 Feb 27, 2019

Choose a reason for hiding this comment

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

@wardpeet Why start with the falsy value in your if rather than the opposite? It saves 1 character by removing the !.

const getProps = ({ isPartiallyCurrent }) => {
  if (isPartiallyCurrent) {
    return { "data-active": true }
  }
  return
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just the code style I use. It's called Return Early. I like to put meaningless returns like error state or null returns at the top and actual code that I should care about in the happy path at the bottom.

Let's not go into the world of micro-benchmarking it's most of the time not worth it. In this example, there will be nothing to win as a minifier just fixes this.

your snippet:

const getProps = ({ isPartiallyCurrent }) => {
  if (isPartiallyCurrent) {
    return { "data-active": true }
  }
  return
}

minification:

const getProps=({isPartiallyCurrent:t})=>{if(t)return{"data-active":!0}};

my snippet:

const getProps = ({ isPartiallyCurrent }) => {
  if (!isPartiallyCurrent ) {
    return
  }
  return { "data-active": true }
}

minification:

const getProps=({isPartiallyCurrent:t})=>{if(t)return{"data-active":!0}};

Copy link
Contributor

Choose a reason for hiding this comment

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

@wardpeet No no, don't worry, I was simply asking out of curiosity. Thanks for the clarification. 😄

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

no breaking things, just adding a few comments. I'll approve anyway

@wardpeet wardpeet added status: awaiting author response Additional information has been requested from the author and removed status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response labels Feb 27, 2019
@DSchau DSchau requested a review from a team as a code owner February 28, 2019 22:07
@DSchau DSchau requested review from a team February 28, 2019 22:07
@DSchau DSchau merged commit ba59663 into gatsbyjs:master Feb 28, 2019
@DSchau DSchau deleted the www/emotion-10 branch February 28, 2019 22:23
@fk
Copy link
Contributor

fk commented Mar 2, 2019

<3 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting author response Additional information has been requested from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants