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-style.css to _s? #1019

Closed
mrwweb opened this issue Sep 16, 2016 · 31 comments
Closed

Add editor-style.css to _s? #1019

mrwweb opened this issue Sep 16, 2016 · 31 comments

Comments

@mrwweb
Copy link
Contributor

mrwweb commented Sep 16, 2016

The editor-style.css file is far and away one of the best ways to improve a site editor's experience (at least in my experience). It makes TinyMCE much WYSIWYGier(TM) and helps users understand the connection between a theme and typography styles.

So in that vein, I'd love to see _s add editor-style.css by default to the theme. Thinking through this, I believe that would involve doing the following:

  1. Add add_editor_style(); to functions.php
  2. Add an editor-style.css file to the theme's root and include a brief description and link to documentation in a comment.
  3. Add an editor-style.scss file to the /sass/ folder that includes the same comments as above AND @imports helpful partials including:
    • everything in /sass/typography/
    • everything in /sass/elements/
    • everything in /sass/modules/
    • normalize
    • all requires mixins and variables partials to make the above compile

I've taken the approach in #3 with my own themes on multiple sites and have found a lot of success in autogenerating a nearly-complete set of editor styles. I don't know the _s SASS setup that well, so some input on that would be useful.

I thought a discussion would be worthwhile before submitting a pull request, so please let me know what you think. As far as I can tell, this hasn't been considered before.

@josephfusco
Copy link
Contributor

Looks like this was considered before but closed on the premise that there are no styles in this starter theme.

There was however no sass implementation when this PR was closed and believe that laying out the sass structure you mentioned could be beneficial as it is not adding in any new styles. I personally use approach 3 you mentioned and always end up manually creating editor-style.scss importing all partials related to themes post styles.

@mrwweb
Copy link
Contributor Author

mrwweb commented Sep 16, 2016

@josephfusco Glad to hear that strategy works for more than just me. Can you find the previous discussions? My initial searching didn't turn up anything.

In theory, I guess a pull could include the same styles in the non-SASS style.css, my only concern there is that it would add a third place that CSS needs to be updated frequently which feels like a losing proposition. I'm also seen people just include their full-site stylesheet as the editor style, but I don't think that works well in practice (it's bloated, and styles often assume markup that isn't in the editor, such as styling .site-content h2).

And just to look at other precedents, every Twenty XXXX default WordPress theme has shipped with an editor-style.css, so this is a clearly a core-blessed best practice.

@josephfusco
Copy link
Contributor

Ref: #225

@valleyspirit
Copy link

I politely disagree with @mrwweb that there is a precedent. This is not a default WordPress theme designed to be loaded up and evaluated for style by hundreds of thousands of people, like a Twenty-[yearXX] theme. This is not even an end-user theme. For me personally, Underscores is no more supposed to improve a site editor's experience than it is to improve a site reader's experience, and I certainly don't want to start seeing readability/legibility styles, complete with fonts, put into the underscores templates. That's not what it is. It's wall framing for your house, not living room paint nor pre-packaged bathroom fixtures. It is not supposed to provide an idealized editor page, any more than it is supposed to provide an idealized full-width content presentation.

I like the current intent of this project, and I hope the team does not add anything else that (for instance...) I would have to rip out (whether entire sass files or sass fragments or css file fragments) to use bootstrap or another front-end aesthetic.

This is an awesome style-agnostic theme skeleton! Yay. Thanks, everyone, for the work. I am only here because I am actually pulling styles out of Underscores today. Don't put any more in, hehe. Pretty please.

@mrwweb
Copy link
Contributor Author

mrwweb commented Sep 29, 2016

Just to clarify my thinking:

  1. I've always viewed _s as a set of best practices meant to create the best possible theme. I think the best possible theme includes an editor-style.css. Just like _s includes a blank screenshot.png, I think there is value in creating a placeholder that a theme author should deal with.
  2. The goal is to not add extra styles, only pull in existing styles from other SASS sheets. My gut says to leave the editor-style.css blank, even though that would mean it's inconsistent with the SASS branch.

Considering those two points together, I think that very much fits in with the "framing" metaphor.

@valleyspirit
Copy link

@mrwweb placeholders would be a great way to propagate best practices. I learned how to style the editor recently from another project based on this one, and I could easily have been taught how to do it properly by Underscores. I think that's very much in line with the framing metaphor.

I agree with your first two (original) points. However, if you go so far as to pull in styles from elsewhere, then I would strongly disagree with your intent, which is styling a skeleton. This is why you see precedent where there is none.

I think the sass/css which is here, should be minimal to achieve a layout that isn't smashed together and that is readable enough to proceed to style. I don't think Underscores' styles should be some minimalist aesthetic that we should also apply to the editor. There should be no aesthetic intent. I think styles here should only exist to the extent that the architecture is defined in the best way. Which would be entirely contrary to pulling in styles from elsewhere to "style" something that is entirely present and usable.

I entirely support the first two thirds of your goals here, but the third pains me greatly as an architect. Why don't we just leave comments in the css/sass file clarifying intent? This would be similar to the internationalization guidance.

@msroberts
Copy link

Could you create an editor-style.scss that just imports modules/alignments and media/media (and any necessary variables and mixins), so that WordPress' built-in image alignment classes and galleries will work the same way they do in the theme?

@josephfusco
Copy link
Contributor

josephfusco commented Sep 29, 2016

We decided not to include translations [#50] beyond the existing _s.pot file, a RTL stylesheet [#263], or editor styles [#225], as they are likely to change during development of an _s-based theme.

Seems like there are some inconsistencies with what is stated in CONTRIBUTING.md and what actually exists in the project.

There is a placeholder rtl in the project but no placeholder editor-styles.css/editor-styles.scss like what is being suggested.

@msroberts Yes something like this:

/* Used to style the TinyMCE editor. */

/*--------------------------------------------------------------
>>> TABLE OF CONTENTS:
----------------------------------------------------------------
# Normalize
# Typography
# Navigation
    ## Links
# Alignments
# Clearings
# Media
    ## Captions
    ## Galleries
--------------------------------------------------------------*/
@import "variables-site/variables-site";
@import "mixins/mixins-master";

/*--------------------------------------------------------------
# Normalize
--------------------------------------------------------------*/
@import "normalize";

/*--------------------------------------------------------------
# Typography
--------------------------------------------------------------*/
@import "typography/typography";

/*--------------------------------------------------------------
# Navigation
--------------------------------------------------------------*/
@import "navigation/links";

/*--------------------------------------------------------------
# Alignments
--------------------------------------------------------------*/
@import "modules/alignments";

/*--------------------------------------------------------------
# Clearings
--------------------------------------------------------------*/
@import "modules/clearings";

/*--------------------------------------------------------------
# Media
--------------------------------------------------------------*/
@import "media/media";

@mrwweb
Copy link
Contributor Author

mrwweb commented Sep 29, 2016

That seems about right. Again, the main goal is to give developers a starting point but not make any decisions for them. (Hence probably the suggested differences between the SCSS and CSS versions.)

I want to take a closer look at @josephfusco's suggestion, but it's definitely headed in the direction I imagined

@karmatosed
Copy link
Contributor

I would vote against this as the editor-style should heavily be generated from styles of the theme. As _s is meant to not be a styled theme just a starting point, it makes a lot of sense to not include one.

@mrwweb
Copy link
Contributor Author

mrwweb commented Dec 25, 2016

@karmatosed, you write:

_s is meant to not be a styled theme just a starting point

The proposed solution in this issue was precisely that:

  1. It did not involve writing any new CSS. (Any output CSS would be duplicate CSS from existing styles that are already seen fit for inclusion in _s.)
  2. This was explicitly about providing a good "starting point" while not making decisions. (This is exactly like the inclusion of screenshot.png and rtl.css. Both files are included with no content but provided because they should be included in a good theme.)

It feels like you misunderstood what I was suggesting—which is understandable given previous Issues and PRs about editor-style.css—but could you please reconsider and respond to the ideas raised here?

Based on your comments, I do not understand why this issue was closed and how including an **empty ** editor-style.css (and skeleton .scss) is inappropriate for _s. The feedback up to this point was both constructive and positive.

@jrfnl
Copy link
Contributor

jrfnl commented Dec 25, 2016

Please reconsider. I too believe that having a skeleton editor-style.css file will be a good thing as it draws attention to the fact that themes should provide one.

@David-Brown
Copy link

+1 for reconsidering this. As noted above, this is an important (arguably critical) element for any custom theme, and placing it into _S is an important starting point for theme developers. What's been suggested here isn't opinionated, it's merely part of the overall architecture of a good theme, which is what _S is intended to be.

@mrwweb
Copy link
Contributor Author

mrwweb commented Dec 28, 2016

Further reason for reconsideration: CONTRIBUTING.md says:

We decided not to include translations [#50] beyond the existing _s.pot file, a RTL stylesheet [#263], or editor styles [#225], as they are likely to change during development of an _s based theme.

A close reading of the above highlights a few things:

  1. "Editor styles" likely means CSS styles for the editor, not an empty/skeleton editor-style.css
    file
    .
  2. Add editor-style #225 was about adding CSS styles and was closed because those styles were rather opinionated and the ticket was veering off-topic.
  3. Underscores appears to have always had an empty rtl.css. This is exactly what I'm proposing for editor-style.css .

Either the proposed change is in scope or the stated reasons for not having an empty starter file need clarification. Either way, CONTRIBUTING.md needs clarification. I'm happy to get the ball rolling, but need confirmation that the PRs will be considered.

Reminder of the point: Including an empty starter file will teach people about Editor Styles, remind developers to include them by making it easier to start, and improve the UX for themes that wouldn't have otherwise had an editor-style.css file (of which I am sure there are many).

Since I can't re-open this issue, can someone with those powers do so? @karmatosed @kovshenin @davidakennedy @philiparthurmoore @obenland

@samikeijonen
Copy link
Contributor

+1 from me.

@davidakennedy
Copy link
Contributor

I wanted to reopen the issue, and explain the general reasons this was closed:

  1. We don’t include editor-style.css files in our themes for WordPress.com. Calypso does not use them.
  2. It’s overhead to add more base styles and maintain them.

It’s always a fine line between going too far with the start of a starter theme, and not having enough. But it sounds like @mrwweb has something else in mind that takes those concerns into account.

Happy to hear those ideas out and see a PR.

@philiparthurmoore
Copy link
Collaborator

@mrwweb I'm for this, actually. Sorry for missing out on your ping. I'd love to see an empty file PR for this, for the same reason that having an empty rtl.css file in place is a good idea.

@obenland
Copy link
Member

The editor's days are numbered with Gutenberg in beta. Theme styles are the new editor styles.

@davidakennedy
Copy link
Contributor

That's a good point, and something I wasn't thinking about. Even though I gave you some feedback on the pull request @mrwweb, it probably doesn't make sense to add this now.

It stinks as I know you spent time of the pull request, so thank you for that. I think it's best we not worry about this now since we'd have to remove it soon anyway. I'm going to close this out – thanks all for the thoughts here.

@mrwweb
Copy link
Contributor Author

mrwweb commented Jun 23, 2017

@obenland can you expand on this (or provide links):

Theme styles are the new editor styles.

This is something I've been wondering about a lot (general feature parity between current editor filters/styles/actions) and haven't seen a lot clear info.

@obenland
Copy link
Member

As far as I understand WordPress/gutenberg#963 there will be a possibility for themes to add support for block styles that then get used on both frontend and the admin.

@mrwweb
Copy link
Contributor Author

mrwweb commented Jun 24, 2017

@obenland @davidakennedy It seems like that's actually far from a foregone conclusion and there will likely have to be at least some way to include basic typography styles unless/until Gutenberg comes a literal front-end editor embedded in theme code. WordPress/gutenberg#1315.

Given that, I think it might still be worth including this both for until then and so people are thinking hard about theme styles affecting the editor.

@davidakennedy davidakennedy reopened this Jun 27, 2017
@davidakennedy
Copy link
Contributor

@mrwweb I'd be open for that. I think a good compromise is to just add an empty file with some very basic styles, a la the rtl.css file.

@obenland
Copy link
Member

They're actively working on an API to make that happen. WordPress/gutenberg#1420
Editor styles will be deprecated before the end of the year so I really don't see a compelling reason to add them at this point.

@davidakennedy
Copy link
Contributor

That's a good point from @obenland. The gain from adding editor styles at this point would be pretty slim in my view, especially since they've never been in _s to begin with. They'd be there for a few months before being removed.

Curious to here you thoughts @mrwweb since you're passionate about this. 😄

@jrfnl
Copy link
Contributor

jrfnl commented Jun 28, 2017

so I really don't see a compelling reason to add them at this point.

What about themes which want to support more than just the latest & greatest WP version ? Some BC for themes would be nice I think.

@obenland
Copy link
Member

What about themes which want to support more than just the latest & greatest WP version ? Some BC for themes would be nice I think.

Not having it in _s doesn't mean theme authors can't add them.

@mrwweb
Copy link
Contributor Author

mrwweb commented Jun 29, 2017

Curious to here you thoughts @mrwweb since you're passionate about this. 😄

Ain't that the truth :)

Broadly, my current understanding of some relevant things about Gutenberg:

  • Unless I'm misunderstanding things, I'm pretty sure it's not intended to be a front-end editor. I can't envision a way one could import a theme into the editor without breaking the editor and/or missing certain styles.
  • Gutenberg still doesn't have a coherent plan for addressing how to apply something like basic theme typography styles to the editor to make it match the theme like editor-style.css does. I'm still thinking that something like editor-style.css is likely. (Adds an API for enqueuing assets to the theme or editor of gutenberg. WordPress/gutenberg#1420 seems to be the most relevant ticket for that right now.)
  • From where I stand, the timeline for Gutenberg feels awfully optimistic.
  • I'm sure many people will either avoid the Gutenberg update at first or there will be a way to use the older editor as a plugin or something.

So if some form of editor styles will exist in the new editor at some unknown point in the future, I see adding editor styles now to be a good first step toward supporting whatever future method exists! I still maintain that it's a major user experience enhancement and that seems worthwhile, even if only for a few months, but adding it also gets people thinking about how to do the same thing in the future.

We've got a nearly-complete patch that makes things better right now and likely could be modified to work again in the future. I don't see the downside.

@mrwweb
Copy link
Contributor Author

mrwweb commented Jul 6, 2017

The latest comment on WordPress/gutenberg#1420 suggests a change in plans back toward something like editor-style.css for themes and plugins.

@philiparthurmoore
Copy link
Collaborator

This thread seems like a long discussion. What's the harm in adding in editor-style.css right now and removing it if necessary later on? (Starter themes evolve; there is no lock in here.) Making assumptions based on the progress of Gutenberg right now doesn't seem that wise.

@davidakennedy
Copy link
Contributor

We sat on this one for quite awhile – sorry about that @mrwweb!

After some discussion on this today among the Theme Team, we're still on the side of leaving editor-style.css out for now. But I'd expect we'll revisit this type of challenge once we start to evolve _s for Gutenberg, which should be soon.

Thanks everyone for the discussion here!

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

No branches or pull requests