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

SDK: use higher level component to scope styles for Calypso components #26852

Closed
wants to merge 16 commits into from

Conversation

simison
Copy link
Member

@simison simison commented Aug 23, 2018

Based on the previous experiment to scope our CSS (#26683). This has a bunch of code from that other PR to namespace styles needed for this experiment.

This automates the other part — injecting the scope CSS selector in components.

Instead of manually adding .calypso to the block's code, we could add a wrapper component that only adds the scope CSS selector to the component. This would require ensuring each component in Calypso does something like this:

import calypsoComponent from 'lib/sdk';
class Card extends React.Component {
  // ...
}
export default calypsoComponent( Card );

Higher order component would be shipped together with blocks and applied in run-time.

Input:

const edit = ( { className } ) => (
	<Card className={ className }>
		<Ribbon>Hidden</Ribbon>
	</Card>
);

Output:

<div class="wp-block-a8c-editor-notes card calypso">
  <div class="ribbon">
    <span class="ribbon__title">Hidden</span>
  </div>
</div>

We can now use scoped CSS that applies only to Calypso components:

.calypso .card {
  border-color: red;
}

Problems

  • Applied in run time
  • Requires changes to all components in Calypso
  • In this example, block outputs html: <div class="card calypso"></div> but CSS is .calypso .card {} — note the space in between. HTML would need to be <div class="calypso"><div class="card"></div></div> for this to work. That can be solved by wrapping the component in additional <div> at the higher level component.

Testing

  1. Build
npm run sdk:gutenberg -- --editor-script=client/gutenberg/extensions/editor-notes/index.js
  1. Grab files from client/gutenberg/extensions/editor-notes/build and load them in Gutenberg.

  2. Observe the Card element has calypso class in it

dmsnell and others added 15 commits August 22, 2018 12:33
In this patch I'm doing two things at once to reach the goal of using
Calypso UI components: pulling in the UI code and pulling in the styles.

As you may note I'm bringing in the entirety of the Calypso styles since
the framework styles are monolithic. This has the immediate drawback
that the stylesheet is over a megabyte in size, but the immediate
advantage that we can import Calypso UI and not have to write it all
ourselves.

If this turns out to be a good path then we will obviously have to
optimize the CSS, which is already in the plans. As the SDK and Calypso
framework improves we should see that bundled CSS drop down to very
small sizes. Currently the framework isn't ready to split apart the CSS
though I'm afraid.
Previously we were pulling in the top-level stylesheet from Calypso
and we were doing so from the top of the extension code. This pulled
in over a megabyte of CSS and required us to be aware of Calypso's
styling and style paths.

In this change I have updated the `Card` and `Ribbon` components so
that they explicitly import their SASS dependencies. This allows us
to pull in _only_ the required style when building the extension.

The output dropped from a megabyte to three kilobytes.
> - configuration.module.rules[2].exclude should be one of these:
>   RegExp | non-empty string | function | [RegExp | non-empty string | function | (recursive) | object { and?, exclude?, include?, not?, or?, test? }] | object { and?, exclude?, include?, not?, or?, test? }

Setting it to `undefined`  would work as well.
@matticbot
Copy link
Contributor

@simison simison changed the title Try/sdk scoping with higher level component SDK: use higher level component to scope styles for Calypso components Aug 23, 2018
Instead of:
```
<div class="component calypso"></div>
```

Do this:
```
<divclass="calypso">
  <div class="component"></div>
</div>
```
@ockham
Copy link
Contributor

ockham commented Aug 24, 2018

Problems

  • Applied in run time
  • Requires changes to all components in Calypso

See my comment on pa9srA-l-p2:

In terms of developer experience, I’d personally like to rule out anything that requires the developer to manually add a scope class — and that includes wrapping components in an HoC that provides that kind of class. (Rationale — this is something a dev shouldn’t simply have to worry about when writing a component, and requiring this kind of manual wrapping is probably easy to miss and thus fragile.)

@simison simison closed this Sep 25, 2018
@simison simison deleted the try/sdk-scoping-with-higher-level-component branch September 25, 2018 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants