Reduce duplicate code across plugins using notify popup

[kirsty-hames]

Subject of the enhancement

Currently the default style for Notify icon buttons is consistent across all plugins however we've duplicated the styling code across the various plugin LESS files.

See Hotgraphic close button in comparison to core Notify close button. The CSS being applied is the same but attached to different selectors.

See references to duplicate styles below:

• Notify targets .notify__btn-icon
• Tutor targets .tutor__btn-icon
• Hotgraphic targets .hotgraphic-popup__controls and .hotgraphic-popup__close

Expected behaviour

The generic Notify button icon selector .notify__btn-icon is only being used in the notifyPopup template.

I'd expect .notify__btn-icon to be used for all Notify icon buttons. This means the default style is set in one place but we still have plugin specific classes to target should you want to style these different - although there's good reason for keeping UI button styling consistent.

Next steps

• Raise issues and PRs for the plugins where the additional selector .notify__btn-icon is applicable.
• Only once plugin PRs are merged should the plugin styles be removed from Vanilla. As we're removing styles that require the compatible plugin? Is this a breaking change for Vanilla or the relevant plugin? Or both?

[StuartNicholls]

@kirsty-hames, as a general question, what are the advantages of having four separate classes/selectors for the close button? As apposed to say, one class/selector called close-btn. The selector can target the icon with a mixin for example.

There seems to be some confusion looking at how this has been implemented generally speaking - across plug-ins, but even in the theme for example

adapt-contrib-vanilla/less/core/notify.less

Line 79 in 142b4fd

border-radius: 50%;

Looking at the naming logic for the classes, I'd expect the radius to apply to the button, rather than in the selector than deals with the icon.

I'm just wondering if it might be better to create a new selector, call it close-btn, then leave those specific selectors (for customisations) then tidy up the less. Thoughts?

[kirsty-hames]

@kirsty-hames, as a general question, what are the advantages of having four separate classes/selectors for the close button? As apposed to say, one class/selector called close-btn. The selector can target the icon with a mixin for example.

I completely agree. On the off chance you did want to do different styles you can still target a parent selector for a particular plugin. Whether we touch the plugin selectors or not a single mixin in the theme would reduce the need for duplicate CSS and resolve the issue raised.

There seems to be some confusion looking at how this has been implemented generally speaking - across plug-ins, but even in the theme for example

adapt-contrib-vanilla/less/core/notify.less

Line 79 in 142b4fd

border-radius: 50%;

Looking at the naming logic for the classes, I'd expect the radius to apply to the button, rather than in the selector than deals with the icon.

I'd also agree. I think this needs to be looked at from a wider UI perspective though (same issue applies to Drawer buttons etc) so I think this can be a separate issue/task. For the purpose of this ticket, attaching the current styles to mixin is a starting point.

I'm just wondering if it might be better to create a new selector, call it close-btn, then leave those specific selectors (for customisations) then tidy up the less. Thoughts?

Yep agree again, leaving the selectors as is avoids any breaking changes/compatibility issues across Vanilla and plugins so I prefer this approach. Thanks for raising. We just need to rethink the mixin name as I raised this issue for Notify icon buttons not just close so would apply to the controls too (back/next) but your suggestion of a mixin is a good way to go for this.

[StuartNicholls]

@kirsty-hames , what I meant was re. the new selector, was create a new selector and add the class to the DOM so, call it adapt-close-btn , but perhaps thats out of scope here. I suggested that as you were suggesting adding notify__btn-icon, to hotgrid etc. this is slightly confusing as its BEM style class but used like utility class if we add it here.

So for now, yeah lets stay away from adding that class to the DOM in other plug-ins and perhaps if we need to add in XXX_btn-icon (so hotgrid_btn-icon in the case of hotgrid) so it (sort of) follows the same BEM naming. And use a mixin like we said above. Otherwise just a mixin.

Added a comment here:

cgkineo/adapt-hotgrid#127 (comment)

All the other stuff about utility classes and rationalising the ones already in use can get raised in another ticket.

What do you think?

[kirsty-hames]

@kirsty-hames , what I meant was re. the new selector, was create a new selector and add the class to the DOM so, call it adapt-close-btn , but perhaps thats out of scope here. I suggested that as you were suggesting adding notify__btn-icon, to hotgrid etc. this is slightly confusing as its BEM style class but used like utility class if we add it here.

So for now, yeah lets stay away from adding that class to the DOM in other plug-ins and perhaps if we need to add in XXX_btn-icon (so hotgrid_btn-icon in the case of hotgrid) so it (sort of) follows the same BEM naming. And use a mixin like we said above. Otherwise just a mixin.

Added a comment here:

cgkineo/adapt-hotgrid#127 (comment)

All the other stuff about utility classes and rationalising the ones already in use can get raised in another ticket.

What do you think?

Lets just go with the mixin. I'd say adding/changing classes is out of scope for this issue. The mixin alone resolves the issue of duplicate styles.

[github-actions]

🎉 This issue has been resolved in version 9.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀