liip / bootstrap-blocks-wordpress-plugin

Bootstrap Gutenberg Blocks for WordPress
https://wordpress.org/plugins/wp-bootstrap-blocks/
280 stars 60 forks source link

Button block style is not applied in editor #114

Closed R33D3M33R closed 2 years ago

R33D3M33R commented 2 years ago

Dear developers,

the button block has two styles primary and secondary, but they are not applied to the editor on selection, because getEditWrapperProps in block.js only returns data-alignment (and not data-style). This means that changes for this select box cannot be previewed.

Please fix this if possible.

Best regards, Andrej

isaumya commented 2 years ago

Hi there, I also agree, this is a great project with so many hooks and filters available to customize it even more. But the only problem with the button block is that on the back end it does not add any bootstrap class, so inside Gutenberg it look really bad despite all bootstrap styles being present to load.

For example, if I add a button block and select the style as Primary on the front end btn btn-primary class gets added but on the backend, none of those gets added to the button, so a result it looks bad.

Please fix this issue, so that the btn class is added on the Gutenberg side as well and then based on the style selection btn-primary or whatever user select gets added too.

P.S.: Thanks for wpBootstrapBlocks.button.styleOptions hook using which users can add all the custom button styles they want. Just fix the above-mentioned issue and the button block will also become perfect. Thanks.

tschortsch commented 2 years ago

You are right the Button block didn't get as much love as the grid components (since we're thinking that those are the ones used most with this plugin).

@isaumya We're not really loading the bootstrap css in the backend that's why we can't use the btn-xy classes in the editor to set the color. We will have to add a color property to the styleOptions which will define how the button should be colored in the backend.

@R33D3M33R We will add the data-style attribute to the block in the next release.

isaumya commented 2 years ago

Hi @tschortsch, Yes, I understand that you are not loading the bootstrap CSS on the backend and trust me that's the best part of the plugin. But I do load a tiny bit of bootstrap on the backend. So, having the ability to add that would be nice. I mean if not by default, maybe a filter, enabling which will add those classes so that in case the dev is loading the bootstrap CSS on the backend themselves they can take advantage of it.

tschortsch commented 2 years ago

@isaumya there are several issues with loading the Bootstrap CSS in the backend. The biggest one is that the HTML structure of the blocks in the editor is pretty different that you would build it with bootstrap directly. This makes it really hard to apply the correct classes there. There might be also version differences in the CSS which then would require you to define the exact version in the backend. Then there are people which modify or extend Bootstrap by themselves which would make it pretty hard to apply the same styles.

tschortsch commented 2 years ago

I released version 4.3.0 (https://github.com/liip/bootstrap-blocks-wordpress-plugin/releases/tag/4.3.0) of the plugin which reflects the selected button styles in the editor. There is also a [data-style]="xy" attribute added to the block itself to make it possible to apply own styles.

isaumya commented 2 years ago

Hi @tschortsch, I would like to inform you that with v4.3.0 the wpBootstrapBlocks.button.styleOptions filter is not taking effect. I've tested on WP v6.1 and with the new update (v4.3.0) none of my added style options shows in the editor. Initially I thought this might be happening due to WP v6.1 and spend some time debugging things. Then I reverted back to v4.2.1 and the custom style options are showing again. So, I can confirm that the issue is happening with v4.3.0 only. Can you please take a look? Due to this issue, can't update to v4.3.0

tschortsch commented 2 years ago

Hmm I just tested on a clean WordPress site and on a clients project and it works in both places. But there is actually a regression that when the wpBootstrapBlocks.button.styleOptions filter is used but no color attribute is set the buttons appear invisible since they don't get a color applied. Maybe that's your issue? I will fix this bug as soon as possible.

tschortsch commented 2 years ago

@isaumya Could you maybe send me the code snippet how you implemented the filter?

isaumya commented 2 years ago

Hi @tschortsch, forget about my code snippet, I literally tried the code snippet present in the example doc, put it in a file editor.js (inside my custom theme) and loaded it using enqueue_block_editor_assets (in functions.php) and there is nothing.

I did it as I initially thought that there might be some bug with my code so I tried the dummy one. Then when I moved to v4.2.1 my code is working perfectly. BTW, here is my actual code (the color option hasn't been added yet as v4.3.0 is not working with the filter).

wp.domReady( () => {
  wp.hooks.addFilter(
    'wpBootstrapBlocks.button.styleOptions',
    'acnam/wp-bootstrap-blocks/button/styleOptions',
    ( styleOptions ) => {
      return [
        ...styleOptions,
        {
          label: 'Infra Red',
          value: 'infra-red'
        },
        {
          label: 'Ocean Blue',
          value: 'ocean-blue'
        },
        {
          label: 'Ocean Blue Dark',
          value: 'ocean-blue-dark'
        },
        {
          label: 'Navy Blue',
          value: 'navy-blue'
        },
        {
          label: 'Charcoal',
          value: 'charcoal'
        },
        {
          label: 'Brown Flat',
          value: 'brown-flat'
        },
        {
          label: 'Green Flat Dark',
          value: 'green-flat-dark'
        },
        {
          label: 'Light',
          value: 'light'
        }
      ];
    }
  );
} );
isaumya commented 2 years ago

Hmm I just tested on a clean WordPress site and on a clients project and it works in both places. But there is actually a regression that when the wpBootstrapBlocks.button.styleOptions filter is used but no color attribute is set the buttons appear invisible since they don't get a color applied. Maybe that's your issue? I will fix this bug as soon as possible.

Actually no. With the filter added in v4.3.0 under the style option, the custom styles don't even show up. Only Primary and Secondary shows up. Nothing else. But as soon as I move to v4.2.1 all the style options I added show up. Only on v4.3.0 none of my custom-added options doesn't show up. Even tried the demo code, still doesn't show up. Still only Primary & Secondary.

Screenshots

Same code and absolutely no changes in editor.js file.

with v4.2.1 v4.2.1

with v4.3.0 v4.3.0

I hope the situation is more clear now.

tschortsch commented 2 years ago

Thanks for the detailed description. Sadly I still can't really reproduce this behavior 🤔 Could you maybe check your browsers console if there is any error related to that? And could you check in the network tab if your custom editor.js file gets loaded correctly? What WordPress version are you using? Till now I checked your issue with 6.1 & 5.5.9.

tschortsch commented 2 years ago

@isaumya one other question is at which point you're adding those filters. We moved the applyFilter call of the button block outside of the render() function so that all filters (of all the blocks) get applied consistently. Your addFilter call should happen before the block applies those filters which means it should be added as early as possible in the JavaScript loading chain. You should also avoid wrapping the addFilter call inside wp.domReady since they don't need to wait till the DOM is mounted (just add the wp-hooks dependency in the wp_enqueue_script() call since you're relying on the wp.hooks object. Could you try this and tell me if it works for you?

isaumya commented 2 years ago

Hi @tschortsch, Thank you so much for all the help. Loading the filter outside of wp.domReady did the trick. Now the filter is working properly and all the custom style options are loading as well. But I have some important feedback (which might also be breaking change if people have already updated their hook for the v4.3.0). Here are the feedbacks:

  1. You should use more appropriate names for the properties inside the object. Honestly color is the worst name as when I saw it for the first time I thought it was talking about the text color instead of the background color as in CSS color refers to the text color. So, you should highly consider renaming the color property to bgColor as that would be more appropriate helping people easily understand that the properly is for background-color and not for text color.
  2. You should still keep the color property but it should point to the CSS color property which is used for text color. This is very important because I also have .btn-light added in the style property. Now in the current situation if I set the background color to #f8f9fa for .btn-light then the button becomes impossible the see as the text color remains white. So, if the current color property refers to text color then for the Light style option I can add both the text color and the background color.

So, things should look something like this:

{
  label: 'Light',
  value: 'light',
  bgColor: '#f8f9fa',
  color: '#000000'
}

This would be the most appropriate way of implementing the style options for the buttons.

tschortsch commented 2 years ago

Happy that your custom styles now work properly. And thanks a lot for your feedback. I totally agree with you that the new property name might not be ideal. Maybe we'll change that in a later version but as you also mentioned this would be a breaking change so we have to be really careful doing that.

isaumya commented 2 years ago

Hi @tschortsch, yes as it's a breaking change, I think this should be pushed to an update ASAP before mass users adopt to v4.3.0 and update their codes. As you know most webmasters don't update the plugins immediately. So, having this change pushed quickly will mean that it will affect a much less number of people compared to pushing it in the future by when a lot more people might be using v4.3.0 and have updated their code. But at the end of the day, the decision is all yours, I just added my 2 cents.

tschortsch commented 2 years ago

I see your point but right now it's also an issue of available time. If you have time to implement this change and provide a pull request we're open to integrate it in the plugin.

isaumya commented 2 years ago

Hey @tschortsch, will editing just the edit.js work? Or do I also have to do anything else?

tschortsch commented 2 years ago

To implement the feature itself theoretically yes. But we also need to update the documentation, the tests and the changelog which is normally the bigger part of the work 😅

isaumya commented 2 years ago

Hi @tschortsch, I'm currently working on the fix as we speak and after testing I will also update the documentation after I test the changes on my end to ensure everything is working. Then I will push the PR. Then you need to update any other tests, can you please do that?

isaumya commented 2 years ago

Hi @tschortsch, I've pushed a PR: https://github.com/liip/bootstrap-blocks-wordpress-plugin/pull/118 Can you please take a look? I've run all the tests and everything is working as expected. Please take a look and merge it if you can. 🙏🏼

tschortsch commented 2 years ago

Hey @isaumya. Thanks for the PR. Version 5.1.0 is released. I renamed the color attribute to bgColor and added a textColor attribute. Like this we could assure backwards compatibility with the deprecated color attribute and avoid a breaking change.

isaumya commented 2 years ago

Hi @tschortsch, Thanks a lot for the update.