WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.49k stars 4.19k forks source link

Add missing class names from sidebar panels #12082

Closed elliotcondon closed 5 years ago

elliotcondon commented 5 years ago

Describe the bug Sidebar panels (Categories, Featured Image, etc) are missing unique class names.

It would be very helpful to developers if all sidebar panels contained a unique class name (or id) for CSS/JS targeting.

It looks like some of the panels (post status, revision) offer this already and it would be great to have consistency throughout the markup.

Screenshots screen shot 2018-11-20 at 10 20 35 am

Additional context

designsimply commented 5 years ago

This was asked in the past and the answer provided was that developers should not rely on CSS selectors to customize panels and that filters and slots should be used instead ( https://github.com/WordPress/gutenberg/issues/6057#issuecomment-427669693 ). May I please ask that you try the solution noted there and if you have a very specific use case similar to the comments on that issue, note them here and we discuss the best route forward.

elliotcondon commented 5 years ago

@designsimply I'm not seeing any "solution" on that comment. Could you please provide some examples of how we can customize these sidebar panels using JS? For example:

  1. How can we hide a panel (display:none;) using JS/CSS?
  2. How can we style a field (wrapping element with label / select) with CSS?
chrisvanpatten commented 5 years ago

@elliotcondon Have you looked at using wp.data to dispatch actions that would hide panels? You can look at the code that powers the "options" modal to see how that works.

chrisvanpatten commented 5 years ago

@elliotcondon Further reading: https://github.com/WordPress/gutenberg/pull/11802

elliotcondon commented 5 years ago

@chrisvanpatten would love to see some code examples please 🙏.

chrisvanpatten commented 5 years ago

@elliotcondon Our notifications must have crossed paths but check #11802 :)

elliotcondon commented 5 years ago

@chrisvanpatten I commented too fast 🤣.

Thanks for the link. I've had a look and can see there are a few functions available including removeEditorPanel(). This function looks to completely remove the panel, including all the controls within it, which will return different results than simply hiding it via CSS.

My goal here is not to get bogged down in specific use cases. Instead, I am asking WordPress to retain the same thinking that went into the previous generation admin. The current "classic editor" is amazing to work with (as a web developer) because it follows simple and intuitive naming conventions / HTML markup.

I don't believe we should ask web developers to refine their HTML/CSS/JS skills for front-end, but then tell them they are "doing it wrong" in the back-end. Web developers are a creative bunch. And even after 8 years working on ACF, I am still amazed at the customizations I see in the admin area/editor.

I hope we can add in these classNames, it really will help a lot of people.

elliotcondon commented 5 years ago

Here is a screenshot of the Gutenberg editor showing that WP core use class names on some sidebar panels for CSS styling. Note the .edit-post-last-revision__panel class. screen shot 2018-11-20 at 3 31 48 pm

This topic is to ask for consistency across all sidebar panels as this would allow all developers to style components in a similar way.

chrisvanpatten commented 5 years ago

@elliotcondon what would be the technical case for hiding a panel via CSS where the JS option wouldn’t be sufficient?

I also think generally we want to discourage people from styling Gutenberg’s built in components, and certainly the editor panels, from a plugin or other integration. Generally the recommendation has been to filter any panels you want to style and provide your own markup/bindings to the API/styles — aka replacing the panels Gutenberg provides rather than modifying them.

elliotcondon commented 5 years ago

@chrisvanpatten It is very clear that you want to discourage people from styling Gutenberg’s built in components. Thanks for your time.

chrisvanpatten commented 5 years ago

@elliotcondon just to be clear I’m only a third party volunteer and my word should not be taken as gospel. I’m a part of the team but don’t represent the opinions of the release leads. My comments are just based on having followed/contributed to Gutenberg for some time and having seen similar questions and topics come up in past issues.

I know it is a bit frustrating because the Gutenberg mentality is often different from previous norms — it is a big mental shift — but seriously if I can help, or help build a good technical case for a change, I am totally happy to do it. I am here to problem solve!

elliotcondon commented 5 years ago

@chrisvanpatten Can you please take a quick look at my screenshot above and let me know your thoughts? WP core add classes to the sidebar panels for CSS styling. Why not add it to all, so we can all style?

elliotcondon commented 5 years ago

The crazy thing here is that I personally don't need this feature. I've just worked in this space for so long, that I am good at predicting what "minor changes" will provide "maximum happiness" to developers.

jasonbahl commented 5 years ago

Ya this seems closed pretty prematurely 🤷‍♂️

pento commented 5 years ago

Thank you for the suggestion, @elliotcondon!

For hiding these panels, you should definitely use the JS API. Any sort of CSS changes won't be reflected in the mobile native block editor, but using the JS API will be.

For other styling, I'm quite wary of encouraging custom styling of individual builtin components. Apart from having the same issue as above, where it would only apply to the web-based editor, there's been a significant amount of effort put into making Gutenberg the base for a WordPress design language. While there's still some ways to go, you can look at the block editor today, and see the consistency across the interface, something that WordPress has historically struggled to do.

Design languages aren't new, plenty of other projects use them: Apple’s Human Interface Guidelines, or Google’s Material Design are great examples. For developers, these kinds of standard design documents allow you to build really nice interfaces without needing a designer to lay everything out for you. For designers, you get to focus on the bigger UX issues, rather than needing to style the same button 100 times.

To bring it back to this particular issue, if there's something in the interface that needs customising, then it probably points towards an issue with the design language itself, which should be addressed.

So, barring any specific use cases for adding these classes, I'm going to leave this issue closed. As the design language (and sidebar extensibility) evolves in subsequent WordPress releases, we should ideally be able to build something that doesn't require plugin developers to make custom tweaks.

alvarogois commented 5 years ago

This need for “code hygiene” is absurd and totally contrary to the most common ways of using WordPress. It seems that there is a need to control at all costs what can or can not be done. Why?!

This is not a project for designers and developers, it is a project for people, and people should be able to screw up if they want. It's not like we're breaking things on purpose.

We are raising complexity in simple things, and this will limit, a lot, what the average user can do. I think it's contrary to the philosophy of the WordPress project.

tomjn commented 5 years ago

There are 2 parts I'd like to unpack a little from a pure React based point of view, the CSS side, and the JS side.

CSS

I don't think this is good practice, and in 99% of cases a developer thinks this is necessary, it isn't. But, there's probably some super obscure thing that requires it anyway that nobody has thought about, so it should be implemented for that

JS

Hooking in via JS on the other hand is a huge no no. Lets work based on the assumption of this code:

jQuery( '#tomshidepostrevisionsbutton').onClick( (evt ) => {
    jQuery( '.edit-post-last-revision__panel' ).removeClass('is-opened');
} );

At first appearances this would remove the is-opened html class, hiding the panel. But that's not the case:

So things will not work as expected, this is because there is a fundamental misunderstanding going on that assumes that latching on to DOM nodes generated by React is a good thing, that's not how things work. Not because somebody declared so, as a style or preference, but because structurally that's not how this works, in the same way that you can't redirect a river to flow up a mountain at a 45* angle by just moving dirt around.

So How Does it Actually Work?

Internally, the panel component has state. It's a JS array that looks similar to this:

this.state = {
    opened: props.initialOpen === undefined ? true : props.initialOpen,
};

https://github.com/WordPress/gutenberg/blob/master/packages/components/src/panel/body.js#L21

If you click on the panel, a function called toggle is called in that component, and opened is flipped from false to true or true to false, and React re-renders the panel.

Why This is Suboptimal, and What Gutenberg Should Actually Do

So what really needs to happen is a way to changed opened to false. In a modern React application, you would store that in a central data store, and dispatch actions/messages such as "OPEN PANEL" or "CLOSE PANEL", but in this case, that's not what's happening here, as it's internal state of a component.

So that's what needs requesting. That the UI's state be stored in a wp.data store where it can be read and updated, not inside a components internal state. Components gain a lot of advantages by being stateless and taking their data from a central store, and amazing things can be done with that, such as scrubbing through the UI with a timeline back and forth, replaying what a user did on multiple devices, and a huge number of debugging options.

Doing it via jQuery or vanilla JS DOM manipulation will never work, or be conflict free, it never has been.

For example in the classic editor you could target the internal textarea and insert arbitrary text. It would mess with the WYSIWYG TinyMCE instance somewhat, it was crude, and it could lead to all sorts of painful conflicts, but it was doable.

Instead, TinyMCE provided APIs that you were meant to use, e.g.:

tinymce.get(“content”).setContent(“my custom content”);

In Gutenberg however, that data is structured and stored in the data store, you dispatch updates as I mentioned before.

So how do we do that? Via wp.data.dispatch( 'core/editor' ). For example:

// clear out all the content
wp.data.dispatch( 'core/editor' ).resetBlocks([]);

// Add a block
let block = wp.blocks.createBlock( 'core/paragraph', { content: 'Hello World' } );
wp.data.dispatch( 'core/editor' ).insertBlocks( block );

Running wp.data.dispatch('core/editor') in the dev tools console also provides a number of block manipulation functions, moving blocks, selecting blocks, inserting, adding, redo, undo, trashing the post, etc

In an ideal world, we would do something similar for the panel.

I strongly recommend that people follow this course by the author of redux, it walks from the very basics building layer on layer to explain how and why things work the way they do, and the problems solved, yet it's a super short course:

https://egghead.io/courses/getting-started-with-redux

edit: We totally do have this for the panel! See comments below

tomjn commented 5 years ago

Additionally:

https://github.com/WordPress/gutenberg/blob/master/packages/components/src/panel/body.js#L75

As the code demonstrates, toggling the HTML class has no bearing on whether the panel is shown or not. The most you can do is make it always hidden even when it's open

alvarogois commented 5 years ago

Sorry @tomjn, the prior comment about design languages and stuff sure looked about sanitation and “the way things should be done”. Now I get it: it doesn't work that way. Thanks for making it clear.

tomjn commented 5 years ago

@alvarogois no worries, I think there's a learning opportunity here for all, and I learnt some things myself. I'm sure I wrote that reply 3x over each time reading code and adjusting ( I didn't know the panels had internal state, and assumed they used a data store, had to rewrite half the thing! )

tomjn commented 5 years ago

A good example of the kind of thing I suggested was actually done in https://github.com/WordPress/gutenberg/pull/11802 as a means of removing panels:

wp.data.dispatch( 'core/edit-post').removeEditorPanel( 'taxonomy-panel-category' ) ;
wp.data.dispatch( 'core/edit-post').removeEditorPanel( 'taxonomy-panel-post_tag' );
wp.data.dispatch( 'core/edit-post').removeEditorPanel( 'featured-image' );
wp.data.dispatch( 'core/edit-post').removeEditorPanel( 'post-excerpt' );
wp.data.dispatch( 'core/edit-post').removeEditorPanel( 'discussion-panel' );

There may even be isEditorPanelEnabled and toggleEditorPanelEnabled

tomjn commented 5 years ago

And it is indeed possible:

wp.data.dispatch( 'core/edit-post').toggleEditorPanelOpened('post-excerpt')

Running that will open and close the excerpt panel

mtias commented 5 years ago

To further @tomjn point, such an API would allow to separate the application state from the current DOM representation, which means we can orchestrate different plugins interacting with better means, as well as future compatibility with the native mobile apps implementation, where otherwise it would be extremely opaque to the environment.

jasonbahl commented 5 years ago

I may be missing something but part of the original ask wasn’t just about the class names that represent state (is-opened, etc) but also giving each panel its own named class, like how the first 2 in the screenshot have .edit-post-post-status and .edit-post-last-revision

These aren’t stateful classes, unless I’m missing something?

Why can’t each panel have consistent classes like this?

I feel like the discussion sidetracked on the stateful stuff, but these have nothing to do with state. 🤔

elliotcondon commented 5 years ago

Thanks @jasonbahl. Indeed, this discussion got very sidetracked.

My original question was never to discuss the .is-opened class, component architecture, or React. It was to simply ask for uniform approach to sidebar panel classes allowing for "potential" CSS styling opportunities.

designsimply commented 5 years ago

The solution I was referring to was to use filters and slots because that was my takeaway from the comment! But I am not a developer and so I thought the link to the handbook in that comment would suffice. Apologies for making that assumption. I will re-open this and tag it with [Type] Help Request to see if someone with more experience than me can help you with examples for those specific requests.

Aside: (this probably isn't right) but I did look through the handbook a bit and I found a section for filters at https://wordpress.org/gutenberg/handbook/extensibility/extending-blocks/#filters and I also saw that blocks.getBlockAttributes should let you parse and manipulate attribute values—this looked like the closest thing to me but I am not sure whether you can do what you need via attributes. You probably already know about those (in which case ignore this part of my comment!).

designsimply commented 5 years ago

Dangit. I had this open in an old tab and missed the entire conversation after Elliot's first reply to me. Sorry for the noise!

/me backs away slowly …

elliotcondon commented 5 years ago

So.... Q2. How can we style a field (wrapping element with label / select) with CSS?

jasonbahl commented 5 years ago

Wait, so this is staying closed? I’m still confused why there can’t be consistent output of styles without the need for filters. Why are the panels treated inconsistently? Still seems like an oversight, not something to offload to site owners/plugin devs.

alijaffar commented 5 years ago

+1 on unique classes or IDs.

zrubinrattet commented 5 years ago

Adding unique classes/IDs seems like a relatively simple/small thing to do that could have a big impact for some projects. If WP is supposed to be this totally free and open platform why ever have a philosophy to restrict users in any cases? I get the material design thing but you can at least override some of that stuff if you're implementing it usually. We're all consenting adults here. I'm with @elliotcondon on this one 100%.

tomjn commented 5 years ago

Has nobody opened a pull request yet? It's all fun piling in in agreement but condemnation and outrage are neither persuasive nor does it write code.

A pull request that actually implements this is the next step for those who want it, until then it's purely academic so let's see some constructive research into how it might be done? Or code!

On Sat, 24 Nov 2018 at 02:18, zrubinrattet notifications@github.com wrote:

Adding unique classes/IDs seems like a relatively simple/small thing to do that could have a big impact for some projects. If WP is supposed to be this totally free and open platform why ever have a philosophy to restrict users in any cases? I get the material design thing but you can at least override some of that stuff if you're implementing it usually. We're all consenting adults here. I'm with @elliotcondon https://github.com/elliotcondon on this one 100%.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WordPress/gutenberg/issues/12082#issuecomment-441337825, or mute the thread https://github.com/notifications/unsubscribe-auth/AADl59QRglGxbh4kLqa0CA3RMxvLqu-qks5uyKxsgaJpZM4YqFc4 .

jasonbahl commented 5 years ago

has nobody opened a pull request yet?

With all due respect, we’re trying to first have the issue not be closed. If the gatekeepers won’t even acknowledge it is in fact an issue, why waste time on a PR? If we can agree that what @elliotcondon was asking originally, that panels should have consistent classes output I think there’s a chance one of us would be able to work on a PR. But if we’re shut down just by presenting the issue, why would we feel encouraged to open a PR?

I’ll give it a shot if it’s an open issue (probably not super soon though because I’ve got family in town)

chrisvanpatten commented 5 years ago

My own personal take is that there are a lot of people in here asking for classes, but I don’t think we’ve seen a single concrete explanation of what the classes would actually be used for, other than “we might need them in the future.”

I think what would help reopen the ticket are some concrete ideas of how the classes would be used, and why the current methods of customisation aren’t sufficient to accomplish those needs. The talk has admittedly gotten a little philosophical, and steering it back to real technical use-cases would be valuable.

tomjn commented 5 years ago

The only example I can think of at the moment is adding company branding to a panels header, which sounds like marketing UI clutter. Everything else can be done by either wrapping the panel contents in a div with a class, or adding per item level classes to the panels contents, which would be more optimal from a CSS standpoint.

As for reopening it, simply stating some variation of "I disagree" is ineffectual, and polarizes things, so you are probably doing more harm for your cause than good. Examples, use cases, and code on the other hand work, e.g. implementing it and showing an example use case. As of yet, nobody has actually made an effective case for this outside of hooking in with JS, which has been shown that it won't work.

realworldev commented 5 years ago

Shouldn't be too difficult to think of various usecases if you have ever seen realworld WordPress used as CMS with sometimes highly customized backend to guide users with absolutely no technical background to publish posts - some would call this 'democratize publishing'.

General usecase: Plugins which customize admin/edit area like Adminimize with 200.000+ active installs.

Realworld usecase (simplified), client requirements:

  1. show headline "Steps 1-5" in workflow steps
  2. mark different steps with different colors
  3. allow adding new categories, but only top level
  4. show short instructions at each workflow step
  5. long top level category names might need more than one line, provide better readability in list
  6. category box should be higher (400px max.) so more of the 250+ categories are visible

Solution for Classic Editor:

#categorydiv::before {
    content: "Step 1/5";
    display: block;
    font-size: 24px;
    color: green;
    margin: .5em;
}
#categorydiv {
    border: 2px solid green;
}
#categorydiv #newcategory_parent {
    display: none;
}
#categorydiv #category-tabs::before {
    content: "Select a maximum of 3 categories here, select 'Slider' for integration in startpage slider.";
    display: block;
    margin-bottom: 1em;
}
#categorydiv ul.categorychecklist > li {
    line-height: 16px;
    margin-bottom: 4px;
}
#categorydiv .categorydiv div.tabs-panel {
    max-height: 400px;
}

Note: "+ Add New Category" has been clicked in both screenshots:

categorydiv

Similar solution for custom taxonomies boxes, #submitdiv ... imagine that yourself.

Overall development time for above requirements: 15 mins.

Used toolchain: Browser inspector, text editor, add_action('admin_head', ..), FTP client.

Solution for Gutenberg?

Maybe somebody can post a simple solution for category box in Gutenberg for above mentioned client requirements, would like to study and learn, thank you.

Besides that:

+1 on unique classes or IDs.

rilwis commented 5 years ago

I'd love to add unique classes or IDs.

One reason is for consistency as @elliotcondon said. I noticed other issues as well:

Take the Page Attributes meta box as an example. The CSS is following BEM style, but we can't find the class for the block, but inside elements. In details, we have only .editor-page-attributes__template, not .editor-page-attributes. So it's kind of the missing a parent class.

Another reason is it's nearly impossible to target a meta box. For example, there's no difference between taxonomies meta boxes. We can't differentiate built-in Category meta box vs. custom taxonomy meta box.

The reason we need to target a meta box is: in our situation, we want to perform an action (to show/hide a custom field) when users select a category or a post format or a page template. Without ability to target to the category meta box, we can't do that.

tomjn commented 5 years ago

Tran, as I described above that won't work, you can't target React elements and attach event listeners like that.

As soon as someone selects a term or unselected a term, or even expands and contracts the panel, your event listeners would need recreating. If the panel is closed there wouldn't even be DOM nodes to match against. Simply changing the block you're in would change the sidebar undoing what your code that targets the Html class did

Instead, you'd need to use the JS APIs available and do it the proper data driven way, either via the data store or the JS WP Hook system that tries to recreate actions and filters

On Wed, 5 Dec 2018 at 09:37, Tran Ngoc Tuan Anh notifications@github.com wrote:

I'd love to add unique classes or IDs.

One reason is for consistency as @elliotcondon https://github.com/elliotcondon said. I noticed other issues as well:

Take the Page Attributes meta box as an example. The CSS is following BEM style, but we can't find the class for the block, but inside elements. In details, we have only .editor-page-attributes__template, not .editor-page-attributes. So it's kind of the missing a parent class.

Another reason is it's nearly impossible to target a meta box. For example, there's no difference between taxonomies meta boxes. We can't differentiate built-in Category meta box vs. custom taxonomy meta box.

The reason we need to target a meta box is: in our situation, we want to perform an action (to show/hide a custom field) when users select a category or a post format or a page template. Without ability to target to the category meta box, we can't do that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WordPress/gutenberg/issues/12082#issuecomment-444420791, or mute the thread https://github.com/notifications/unsubscribe-auth/AADl53jVNntanrk9h5RB1iiLZBRO_KYyks5u15PagaJpZM4YqFc4 .

elliotcondon commented 5 years ago

Hi @tomjn

jQuery event listeners can be added to a parent element in a way that allows for changing inner elements like so:

$(document).on('click', '.category-items input', function( e ){
   // do something
});

That said, I think we are all happy to use the new JS APIs available. The only problem is that we can't find any resources.

Can you please provide a code example on how we can "listen" for a change of a post attribute. For example, if the user selects "Category 2" (taxonomy term), I would like to run some custom JS. How can we do this?

Thanks in advance.

tomjn commented 5 years ago

You wouldn't listen to the UI, you would listen for the change to the internal data structure.

Also don't forget I'm just another commenter, my daily job is code review. I don't speak in an official capacity so I don't know the ins and outs of developer docs though I know some are in progress, I don't know their state

I do know that the JS hooks stuff was on the core make blogs and their APIs is as close to the PHP equivalent as possible, and that the data store is on GitHub with a tonne of markdown docs, and referenced in the handbook too.

On Wed, 5 Dec 2018 at 10:08, Elliot Condon notifications@github.com wrote:

Hi @tomjn https://github.com/tomjn

jQuery event listeners can be added to a parent element in a way that allows for changing inner elements like so:

$(document).on('click', '.category-items input', function( e ){ // do something });

That said, I think we are all happy to use the new JS APIs available. The only problem is that we can't find any resources.

Can you please provide a code example on how we can "listen" for a change of a post attribute. For example, if the user selects "Category 2" (taxonomy term), I would like to run some custom JS. How can we do this?

Thanks in advance.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WordPress/gutenberg/issues/12082#issuecomment-444430551, or mute the thread https://github.com/notifications/unsubscribe-auth/AADl5yzk-V6IIbWUDiOg1diYxSyaigIGks5u15sWgaJpZM4YqFc4 .

rilwis commented 5 years ago

I agreed with @elliotcondon that we can listen to the document and provide extra action when something changed.

That sounds familiar with the hooks in PHP. So, the question is, if we have to do with React, does Gutenberg provides these kinds of hooks so we can listen to?

For example, here is the source code for post_format dropdown. And there's no such hook.

Anyway, listening to change event is one use case. And you can argue from the point of React or how JS works in Gutenberg (although it's not really supported yet). But the main point of this issue is about just CSS classes and it has nothing to do with the event or data source. It's more for styling or customizing the editor screen.