Use AppButton with extracted Button component

[ecgan]

Changes proposed in this Pull Request:

Closes #1765, and partly addresses #1391.

In this PR, we move from using bundled Button component to using extracted Button component from the @wordpress/components package, and remove the usage of @import "node_modules/@wordpress/components/src/button/style"; in _gutenberg-components.scss.

All <Button> usages are now changed to <AppButton>, including the usages in the external-components directory. AppButton is essentially a wrapper around Button. In the future, if there are breaking changes in Button, we just need to apply the fix in AppButton and it should work throughout the whole plugin.

In _gutenberg-components.scss file, there were some hacks or fixes under .components-button rule. I tested them and they are different from the default @wordpress/components button style. The video recording below shows the difference of the button styles in WooCommerce Marketing page and in GLA page:

Sample code for testing button styles used in the video below

<AppButton variant="primary" isDestructive disabled>
	primary destructive disabled
</AppButton>
<AppButton variant="tertiary" isDestructive>
	tertiary destructive
</AppButton>
<AppButton variant="tertiary" isDestructive>
	tertiary destructive hover not disabled
</AppButton>
<AppButton
	variant="link"
	href="https://www.example.com"
	isDestructive
>
	link destructive focus
</AppButton>
<AppButton
	variant="link"
	href="https://www.example.com"
	isDestructive
>
	link destructive focus not disabled
</AppButton>
<AppButton variant="link" href="https://www.example.com">
	link
</AppButton>

demo-pr-button-styles.mov

To maintain UI appearance in GLA, I have moved the above button styles to AppButton CSS.

For the components-panel__body-toggle's padding-right fix, it seems like it is not needed, so I have removed it. It was introduced in 2228fa5 (#1708) (heads up to the PR author @eason9487). Screenshot below shows the panel without the padding-right fix, and it appears to be working as expected without UI issue:

With the button style no longer bundled and needed, this fixes the CSS issue in non-GLA pages mentioned in #1765.

Screenshots:

Screenshot of WooCommerce Marketing page, without button style conflict from GLA:

Detailed test instructions:

1. Buttons in GLA should continue to appear the same as before, and work as expected.
2. Enable WooCommerce Multichannel Marketing experience in WooCommerce Settings > Advanced > Features page: /wp-admin/admin.php?page=wc-settings&tab=advanced&section=features
3. Go to WooCommerce Marketing page: /wp-admin/admin.php?page=wc-admin&path=%2Fmarketing
4. The expand/collapse button in "Discover more marketing tools" card should appear as normal size, not in tiny size.

Additional details:

Changelog entry

Dev - Use extracted Button component from @wordpress/components package.

[eason9487]

Thanks for the fix. Tested locally and works well. Left a minor comment for another removable workaround.

For the components-panel__body-toggle's padding-right fix, it seems like it is not needed, so I have removed it.

💯

I believe the upstream should have fixed this problem. Thanks for improving this.
Ref: https://github.com/WordPress/gutenberg/blob/%40wordpress/components%4019.17.0/packages/components/src/panel/style.scss#L81

---

• Move all button-related CSS styles under the .gla-admin-page CSS class [...] for scoping the GLA required adjustments.

The reasons I suggested moving button-related styles under the .gla-admin-page CSS class were:

• Button component is also used by @wordpress/components and @woocommerce/components, and these Buttons are DEWPed by the libraries themselves. So, some buttons like the X on a modal won't have the .app-button CSS class. In other words, the button fixes in GLA won't be applied to these buttons.
  
• Preserve the flexibility of using the Button of @wordpress/components directly in the future.
• The codes within the js/src/external-components directory would be a bit nicer to keep the same implementation with their copy as possible and only add needed fixes or features in GLA. Although adding extracted/ would also make it inconsistent with the copy, at least it's clear that it's only because Button is simply externalized. Changing it to AppButton adds another extra mental burden of thinking about whether AppButton is a necessary change for these components.

For the first reason, since these button style fixes were moved under the .app-button CSS class, it makes their application scope smaller than expected, which is to all Buttons in the entire GLA pages.

I guess it's probably not causing any problems at the moment, but I'd like to double check if any of the buttons mentioned in the first reason had the fixes applied to them but are now not applied to them?

[ecgan]

@eason9487 , thanks for the review! 😃

I believe the upstream should have fixed this problem. Thanks for improving this.
Ref: WordPress/gutenberg@%40wordpress/components%4019.17.0/packages/components/src/panel/style.scss#L81

Related to my comment #1834 (comment) about WP L-2 version support policy, FYI I double checked and the same style is also in https://github.com/WordPress/gutenberg/blob/%40wordpress/components%4019.2.3/packages/components/src/panel/style.scss#L78, so I guess we are good.

• Button component is also used by @wordpress/components and @woocommerce/components, and these Buttons are DEWPed by the libraries themselves. So, some buttons like the X on a modal won't have the .app-button CSS class. In other words, the button fixes in GLA won't be applied to these buttons.
  [screenshot of modal with close button]

For these components, I'd say it would be better to create wrapper component around them (e.g. we already have AppModal component that wraps Modal component), and put the CSS in the wrapper component (AppModal CSS would have the hacks for the close button in Modal component).

• Preserve the flexibility of using the Button of @wordpress/components directly in the future.

Personally, I start to think that "using the Button of @wordpress/components directly" may not be a good idea, mainly because of the L-2 version support policy. Imagine if we need to have different CSS hacks in .gla-admin-page for Button component in version L-0, L-1 and L-1, and also hacks for Buttons in Modal, Panel etc.

Having an AppButton wrapper around the Button component (and AppModal etc) gives us a level of control that is easier to reason about and maintain.

This is not to say we must not use Button of @wordpress/components directly. We can, with this "rule": we want the Button that is exactly the same as the one provided by the WP environment. This means accepting the risk that comes with it, e.g. the Button may look different across versions. If we want to modify the component so that it is within our control, then we should do it with a wrapper.

• The codes within the js/src/external-components directory would be a bit nicer to keep the same implementation with their copy as possible and only add needed fixes or features in GLA. Although adding extracted/ would also make it inconsistent with the copy, at least it's clear that it's only because Button is simply externalized. Changing it to AppButton adds another extra mental burden of thinking about whether AppButton is a necessary change for these components.

Yeah, that was a dilemma that I had, I wasn't really sure whether it is wise to do that, that's why I made the changes as a separate commit in c8dadd7 (#1834), so that we can easily revert it if we want.

I chose to do the change because I was favoring consistency across the whole repo, to have only one line of import { Button } from 'extracted/@wordpress/components'; inside AppButton in the whole repo. It is easier to inform future developers to "Always use AppButton, not Button".

For the first reason, since these button style fixes were moved under the .app-button CSS class, it makes their application scope smaller than expected, which is to all Buttons in the entire GLA pages.

Yes, that is expected, and I think that is better. Previously, we have the style in _gutenberg-components.scss which is like a global site-wide style that affects everything. If we put it into .gla-admin-page, it is like a global plugin-wide style within GLA. IMO, these global styles make things a bit more difficult to reason about and maintain. If we put them into wrapper components, it is easier.

I'd like to double check if any of the buttons mentioned in the first reason had the fixes applied to them but are now not applied to them?

I think, the thing is, we wouldn't know for sure? I mean, for example, how do we know the rule &.is-tertiary.is-destructive is originally written and meant to apply to the button in Modal, Panel... etc...? And that is the problem of using global styles like this...? 😅

[eason9487]

Thanks for the explanation in #1834 (comment).

• Button component is also used by @wordpress/components and @woocommerce/components, and these Buttons are DEWPed by the libraries themselves. So, some buttons like the X on a modal won't have the .app-button CSS class. In other words, the button fixes in GLA won't be applied to these buttons.
  [screenshot of modal with close button]

For these components, I'd say it would be better to create wrapper component around them (e.g. we already have AppModal component that wraps Modal component), and put the CSS in the wrapper component (AppModal CSS would have the hacks for the close button in Modal component).

Unfortunately, I'm not pretty sure creating wrapper component(s) would be better. For example, to broadly fix the support of hidden attribute for Button in GLA, all that needs to be done is to add the following style to the _gutenberg-components.scss file.

.gla-admin-page {
  .components-button[hidden] {
    display: none;
  }
}

Instead, the way of creating wrapper component will need to:

1. Search for all components in use that import Button across @wordpress/components and @woocommerce/*.
   • Not only the @woocommerce/components is the searching target because like @woocommerce/customer-effort-score also uses Button.
2. If there are components using Button that are not currently wrapped in GLA,
   1. Create wrapper components for them.
   2. Replace all direct uses with the wrapper components.
   3. For example, Tag, Pannel, DropdownButton, FilterPicker, Chart, Table, CustomerEffortScore and etc., will be the components that need to be wrapped.
3. Duplicate this style fix or style import in all related wrapper components.
4. Note down somewhere to remind devs to use the wrapper components rather than using them directly. Moreover, these rules must also be kept in reviewers' minds for future code reviews.

Maintainability is also significantly different:

• When adjusting, it will need to update all duplicates of the style fix.
• When removing, it will need to remove all duplicates of the style fix and further consider whether to revert the replacements of the wrapper component made when fixing.

Maybe I overlook some reasons that make it better?

---

• Preserve the flexibility of using the Button of @wordpress/components directly in the future.

Personally, I start to think that "using the Button of @wordpress/components directly" may not be a good idea, mainly because of the L-2 version support policy. Imagine if we need to have different CSS hacks in .gla-admin-page for Button component in version L-0, L-1 and L-1, and also hacks for Buttons in Modal, Panel etc.

Not sure what the exact case would be, but the following example should work.

.gla-admin-page {
  .components-button {
    // General fix goes here
  }

  .components-panel {
    // General fix goes here

    .components-button {
      // Specific fix for Button in Panel goes here
    }
  }
}

Having an AppButton wrapper around the Button component (and AppModal etc) gives us a level of control that is easier to reason about and maintain.

Probably I missed some points of view. However, as in the first part of this reply, I don't quite sure that this will be easier to maintain. Instead, it would bring worse maintainability.

This is not to say we must not use Button of @wordpress/components directly. We can, with this "rule": we want the Button that is exactly the same as the one provided by the WP environment. [...]

I'm not sure what situation would require using a AppButton having a compatible fix and also using a Button exactly the same as WP environment but without that fix at the same time, could you offer a practice example?

If we want to modify the component so that it is within our control, then we should do it with a wrapper.

Yes, using a wrapper for the needed things like adding the loading indicator and related styles would be good. But the concern here is about the style workarounds.

---

• The codes within the js/src/external-components directory would be a bit nicer to keep the same implementation with their copy as possible and only add needed fixes or features in GLA. Although adding extracted/ would also make it inconsistent with the copy, at least it's clear that it's only because Button is simply externalized. Changing it to AppButton adds another extra mental burden of thinking about whether AppButton is a necessary change for these components.

I chose to do the change because I was favoring consistency across the whole repo, to have only one line of import { Button } from 'extracted/@wordpress/components'; inside AppButton in the whole repo.

When the entire @wordpress/components can be externalized, it will need to remove the extracted/ for them in the codebase. Since this process is simple, IMO, having many lines of import { Button } from 'extracted/@wordpress/components'; is not a concern.

It is easier to inform future developers to "Always use AppButton, not Button".

I would say this would be more close to implying or inferring, not informing. Otherwise, this PR would not need to do so much with these replacements of Button to AppButton if the AppButton had ever "informed" devs well in the past.

---

For the first reason, since these button style fixes were moved under the .app-button CSS class, it makes their application scope smaller than expected, which is to all Buttons in the entire GLA pages.

Yes, that is expected, and I think that is better.

I couldn't understand why having compatible fixes in AppButton only but not having them in Button is an expected and better result. Could you clarify it?

Previously, we have the style in _gutenberg-components.scss which is like a global site-wide style that affects everything. If we put it into .gla-admin-page, it is like a global plugin-wide style within GLA.

Placing the plugin-wide style fixes under .gla-admin-page in _gutenberg-components.scss is exactly one of the important and main purposes of creating that scoping CSS class.

IMO, these global styles make things a bit more difficult to reason about and maintain. If we put them into wrapper components, it is easier.

As in the first part of this reply, sorry, I can't agree with this point of view. In particular, this approach requires the creation of a dozen wrapped components, and these efforts don't look easy at all. And, wrapping non-basic-level components like FilterPicker, Chart, and Table, just in order to fix a Button style problem inside them, looks impractical and code smells.

I'd like to double check if any of the buttons mentioned in the first reason had the fixes applied to them but are now not applied to them?

I think, the thing is, we wouldn't know for sure? I mean, for example, how do we know the rule &.is-tertiary.is-destructive is originally written and meant to apply to the button in Modal, Panel... etc...?

Yes, we probably don't remember the original purpose. Considering that if these fixes were placed in _gutenberg-components.scss rather than AppButton, doesn't this infer that they were expected to be applied to Button on all GLA pages? The unexpected thing is they pollute other non-GLA pages.

And, the fixes applied to Button on GLA pages have been there for almost a year to over two years (AppButton was not even created yet two years ago), so it could be considered they've been tested over time at least.

Given a comparison table:

CSS selector \ Scope	AppButton on GLA pages	Button on GLA pages	Button on other pages
Global (before)	✔️ ⭕	✔️ ⭕ *	✔️ ❌
AppButton (this PR)	✔️ ⭕	✖️ ❓	✖️ ⭕
.gla-admin-page	✔️ ⭕	✔️ ⭕ *	✖️ ⭕

✔️ Applied
✖️ Not applied
⭕ Expected
❌ Not expected
* At least it works well for 1 - 2 years

The unknown change marked as❓is the blocker for this PR, in addition, this change goes beyond the issue that this PR was probably intended to fix: the Button styles in GLA affect other non-GLA pages, which is the last column of the above table.

Therefore, if this approach is to be adopted in this PR, I would like to clarify whether this change has any extra impact. And yes, that probably means it needs to find and verify whether there are DEWPed Buttons that match these CSS selectors and still need those moved fixes. If not, the current implementation in this PR can proceed as well. But if there are, I'm still not quite keen on actually creating wrappers for those components, and then looping those components to the same verification processing recursively as there are fixes for other components in _gutenberg-components.scss.

And that is the problem of using global styles like this...?

That's why .gla-admin-page is presented for limiting the scope to GLA pages only.

[ecgan]

@eason9487 , thanks for the detailed explanation! 🙏

I'm gonna focus on the following one first, so that we can proceed and move this PR forward: 😄

Given a comparison table:

[...]

The unknown change marked as❓is the blocker for this PR, in addition, this change goes beyond the issue that this PR was probably intended to fix: the Button styles in GLA affect other non-GLA pages, which is the last column of the above table.

Thanks so much for the table, I think I see your point now, and yeah I agree it does go beyond the issue, I was going far ahead of myself, sorry about that. 😅

I will look into moving those CSS from AppButton into the .gla-admin-page and testing it out.

[ecgan]

@eason9487 , I have made a few more commits in this PR.

In b562321 (#1834), I reverted the changes I did on the external-components. I figured that since you are the main code owners and maintainers for this plugin, I should really put your opinions and preferences first over mine. 😄

In 1237f4d (#1834), in external-components, I changed the Button import from @wordpress/components to extracted/@wordpress/components.

Three of the external-components files have eslint warnings that say "node_modules/@wordpress/components/build-module/index.js' imported multiple times. eslint import/no-duplicates", because we have imports from both @wordpress/components and extracted/@wordpress/components. See screenshot below:

In my VS Code editor, if I do an "autofix upon save", it will automatically change the line to import { Modal, KeyboardShortcuts, Button } from '@wordpress/components';, i.e. the "extracted" is removed. I manually saved the file without autofix.

I checked the GLA code base and these three external-components are the only places where we have both @wordpress/components and extracted/@wordpress/components imports. Not sure if you are aware of the above eslint warning and autofix behavior, so just want to point it out.

In 3bc6fbb (#1834), I moved the .components-button hacks from AppButton CSS to .gla-admin-page.

With the above commits, I tested around in GLA and also in WooCommerce Core, it seems like things are working as expected and it fixes issue #1765.

This is ready for another round of review. Let me know what you think on these. 🙂 🙏

[eason9487]

Thanks for the adjustment and consideration. LGTM.

Three of the external-components files have eslint warnings that say "node_modules/@wordpress/components/build-module/index.js' imported multiple times. eslint import/no-duplicates", [...]. Not sure if you are aware of the above eslint warning and autofix behavior, so just want to point it out.

Thanks for pointing out this. It's new and I opened #1844 to solve it.