Skip to content

Conversation

@kyledurand
Copy link
Member

@kyledurand kyledurand commented Nov 23, 2021

WHY are these changes introduced?

First part of #4605

Diff check between previous css build (spacing changes are expected)

This will introduce pxtorem to main. Then we can remove rem() from our public api in v8

WHAT is this pull request doing?

Adds pxtorem to our postcss config

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

@kyledurand kyledurand self-assigned this Nov 23, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 23, 2021

size-limit report

Path Size
cjs 166.15 KB (0%)
esm 96.71 KB (0%)
esnext 143.41 KB (+0.21% 🔺)
css 34.35 KB (+0.53% 🔺)

@kyledurand kyledurand marked this pull request as ready for review November 24, 2021 19:31
@caution-tape-bot
Copy link

👋 It looks like you're updating JavaScript packages that are known
to cause problems when duplicated.

You can deduplicate them with the yarn-deduplicate utility:

npx yarn-deduplicate --packages graphql react react-dom webpack
npx yarn-deduplicate --scopes @shopify @apollo

A duplicate React version may cause an invalid hook call warning.

React context providers usually use module-scoped globals as their
default context values. Multiple versions of such packages will yield
multiple global instances, resulting in oblique runtime errors.

@kyledurand
Copy link
Member Author

Here's a diff of the changes to our build file: https://www.diffchecker.com/SUs9t8LC

The spaces between {} are expected it looks like going from the link @BPScott posted for updating loom

@Shopify Shopify deleted a comment from BPScott Nov 24, 2021
Comment on lines 1 to 6
module.exports = {
rootValue: 10,
replace: true,
propList: ['*'],
selectorBlackList: [],
};
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@kyledurand kyledurand changed the title Add pxtorem postcss plugin Add postcss-pxtorem dependency Nov 24, 2021
@kyledurand kyledurand requested a review from BPScott November 24, 2021 23:13
Copy link
Member

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

Nice clean up + functionality

@aveline
Copy link
Contributor

aveline commented Nov 24, 2021

Will this convert all px to rem? There are valid cases when we would want to keep px values. What does it look like if we want to specify certain values remain px?

eta

Currently, the easiest way to have a single property ignored is to use a capital in the pixel unit declaration.

I guess that works, but feels a bit "magic"

@alex-page
Copy link
Member

alex-page commented Nov 25, 2021

@aveline it definitely is magic.

I hope this feels like autoprefixer. People understand that the output changes but they do not have to write the repetitive code. We also don't need to support a public function. If we can be more opinionated by whitelisting the properties I would be happy to do that.

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Haven't validated the diff output on account of all the whitespace changing but code changes seem reasonable.

It miiiiight be worth splitting out the postcss updates into a separate PR so then it's easier to see "what has changed by adding pxtorem" rather than 100% of the diff being those whitespace differences from the postcss update drowning out any changes as a result of pxtorem. But if you're confident in output then whatever

@kyledurand
Copy link
Member Author

As per Ben's suggestion here's the diff that omits the spacing change https://www.diffchecker.com/GT3piigi

I've looked through it and it's mostly changes to borders, border radii, outlines, visually hidden and a few spacing tokens. I've tophatted things that had potential to look different and all seems good

@kyledurand kyledurand merged commit 7f328ed into main Nov 25, 2021
@kyledurand kyledurand deleted the add-pxtorem-post-css branch November 25, 2021 16:26
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.

4 participants