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

Add editor styles entry point #617

Merged
merged 9 commits into from
Apr 8, 2021
Merged

Add editor styles entry point #617

merged 9 commits into from
Apr 8, 2021

Conversation

coreymcollins
Copy link
Contributor

@coreymcollins coreymcollins commented Mar 26, 2021

I don't think this closes any issue, but it's related to a couple of things noted in #614.

DESCRIPTION

Adds a new entry point for editor styles in /src/editor/editor.js.

This also updates our npm scripts from using our wds-build and wds-start scripts to wp-scripts so that we can easily modify our entry points:

    "build": "wp-scripts build src/index.js src/editor/editor.js && npm run theme",
    "build:editor": "wp-scripts build src/editor/editor.js",
    "start": "wp-scripts start src/index.js src/editor/editor.js",

This does increase compiling times when running npm run start on my machine from around 2600ms on main to around 5000ms in this branch. It'd be great to reduce that number as much as possible, of course, if we can.

Since editor styles are added through theme support hooks, we don't need to worry about wrapping our Tailwind or theme styles in a tag to scope them – this happens naturally:
editor-styles-wrapper class wrapping our styles

SCREENSHOTS

terminal screenshot
updated header styles
header styles on the frontend
header styles on the backend

OTHER

  • Is this issue accessible? (Section 508/WCAG 2.0AA)
  • Does this issue pass all the linting? (PHPCS, ESLint, SassLint)
  • Does this pass CBT?

STEPS TO VERIFY

  1. Checkout the branch
  2. Run npm install
  3. Run npm run build to do a full build and check your editor and frontend styles
  4. Run npm run build:editor to ONLY compile editor styles. NOTE: this ONLY compiles editor styles and NOT styles on the frontend. We may not even need this, but it is useful for testing so if we decide to go this route we could keep it in until we want to merge then yank it out.
  5. Run npm run start and/or npm run start:sync to start the watch process and make some changes, then check the editor and frontend. Compile times did not differ for me running the basic start versus the Browsersync start:sync.

DOCUMENTATION

I think probably, yes – we'll want to mention the update to our npm scripts and how we're including editor styles.

Assigning to @gregrickaby for review but also cc @michealengland to test out and compare to his work in another branch. We may be able to combine some thoughts around this to get a more efficient process running.

@coreymcollins
Copy link
Contributor Author

Okay, big update on everything here so I'm just going to do a whole PR review below.

tldr: we don't actually need any extra entry points because loading the styles in the backend via add_editor_style automatically scopes the styles to the editor. So, doing add_editor_style( 'build/index.css' ); and just using the same index.css file we're already generating works without colliding with other global WP admin styles.

We'll want to test this to make sure there aren't any other conflicts!


DESCRIPTION

Adds our index.css file as the editor styles via add_editor_style( 'build/index.css' ).

We don't need to generate any other stylesheets because add_editor_style will scope the styles to the editor itself.

The only addition we've had to make so far is this in our _globals.scss file:

.block-editor-writing-flow {
	@apply text-base leading-normal;
}

This is because the WP admin 'common.css file sets a fixed font size and line-height on the body tag, so we're targeting the child container of .editor-styles-wrapper to reset these values.

The bonus of doing things this way is that we don't need to do any extra compiling, so our build times don't change at all.

SCREENSHOTS

terminal screenshot
editor styles
frontend styles

OTHER

  • Is this issue accessible? (Section 508/WCAG 2.0AA)
  • Does this issue pass all the linting? (PHPCS, ESLint, SassLint)
  • Does this pass CBT?

STEPS TO VERIFY

  1. Checkout the branch
  2. Run npm install
  3. Run npm run build to do a full build and check your editor and frontend styles
  4. Run npm run start and/or npm run start:sync to start the watch process and make some changes, then check the editor and frontend. Compile times did not differ for me running the basic start versus the Browsersync start:sync.

DOCUMENTATION

Probably yes still, just to mention the use of add_editor_style and maybe the process to how we arrived here.

Copy link
Contributor

@gregrickaby gregrickaby left a comment

Choose a reason for hiding this comment

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

👏🏻 👏🏻

Everything works beautifully on my end. Thanks @coreymcollins

@coreymcollins
Copy link
Contributor Author

Thanks! I think we'll still want to do some testing to make sure there aren't any other style collisions but I think functionally this makes sense (so far).

I've noticed some WDS Block styles and functionality aren't coming through fully, like the accordion, so there may be some additional global admin styles to override.

@coreymcollins
Copy link
Contributor Author

Though now that I'm thinking about it – we should probably worry about testing/fixing styles for the WDS Blocks blocks in the plugin rather than the theme? I think I was still holding onto the mindset of the blocks living in the theme like they used to.

@coreymcollins coreymcollins merged commit 7efebac into main Apr 8, 2021
@coreymcollins coreymcollins deleted the feature/editor-styles branch April 8, 2021 14:37
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.

2 participants