Update: replace notify button icon duplicate styles with mixin (fixes #443) #448

kirsty-hames · 2023-06-15T15:43:00Z

Fixes #443

The default style for Notify icon buttons is consistent across all plugins however the styles are duplicated across the various plugin LESS files.

Vanilla instances include:

Notify popup and Notify push
Hotgraphic popup
Tutor popup

This PR wraps the existing icon button styles within a mixin and applies the mixin in the various plugin LESS files to replace duplicated code.

As the mixin is specific to Notify, the mixin remains in notify.less.

swashbuck

👍

cahirodoherty-learningpool

👀

StuartNicholls · 2023-06-20T10:52:37Z

less/plugins/adapt-contrib-hotgraphic/hotgraphicPopup.less

  &__controls,
  &__close {
-    margin: @btn-margin / 2;
-    padding: @btn-padding / 2;
-    background-color: @notify-icon;
-    color: @notify-icon-inverted;
-    border-radius: 50%;
-
-    .no-touch &:not(.is-disabled):not(.is-locked):hover {
-      background-color: @notify-icon-hover;
-      color: @notify-icon-inverted-hover;
-      .transition(background-color @duration ease-in, color @duration ease-in;);
-    }
+    .notify-btn-icon;
  }


Some small things, perhaps for another PR?

I'm not sure about using the same mixin for both the controls and the close button. I'd imagine these may want to be styled differently. For example, good UI/UX might say you don't really want to give the close button and the nav buttons equal prominence. Suggested here

Additionally, the close button isn't ever disabled/locked so thats also a bit confusing.

Note: We also might want to make the border-radius a less variable? (defo a new PR)

Valid points thanks @StuartNicholls and I agree. As the current styling of controls and close buttons uses the same variables and styles I've maintained this as the purpose of this PR was to just remove the duplication. I think border-radius and controls button styling can both be raised as new issues/PRs.

Cool, the only thing is, do we need a different target in the templates? Fine for a separate PR here but maybe need to rationalise the classes in templates. I'll put a note here

Just picking this back up again. I'll merge this PR as it satisfies the issue raised, to reduce duplicate code across plugins. I think the points made in this discussion can be tackled in separate PRs @StuartNicholls.

I'm not sure about using the same mixin for both the controls and the close button. I'd imagine these may want to be styled differently. For example, good UI/UX might say you don't really want to give the close button and the nav buttons equal prominence. Suggested adaptlearning/adapt-contrib-core#386

Additionally, the close button isn't ever disabled/locked so thats also a bit confusing.

I think both of these will be part of the wider Button review in adaptlearning/adapt-contrib-core#386

We also might want to make the border-radius a less variable?

I'll raise a new issue off the back of this. See #451 .

# [9.7.0](v9.6.16...v9.7.0) (2023-07-04) ### Update * replace notify button icon duplicate styles with mixin (fixes #443) (#448) ([bf2f2e6](bf2f2e6)), closes [#443](#443) [#448](#448)

github-actions · 2023-07-04T15:39:17Z

🎉 This PR is included in version 9.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

kirsty-hames added 4 commits June 15, 2023 16:24

create mixin for notify icon button styles

09a46f2

replace notifyPush close button styles with mixin

484fd1a

replace hotgraphicPopup icon button styles with mixin

5d0bb32

replace tutor close button styles with mixin

9bc0031

kirsty-hames requested review from StuartNicholls and swashbuck June 15, 2023 15:46

kirsty-hames self-assigned this Jun 15, 2023

kirsty-hames added the enhancement label Jun 15, 2023

kirsty-hames mentioned this pull request Jun 15, 2023

Update: add generic .notify__btn-icon class (fixes #126) cgkineo/adapt-hotgrid#127

Merged

swashbuck approved these changes Jun 16, 2023

View reviewed changes

cahirodoherty-learningpool approved these changes Jun 19, 2023

View reviewed changes

kirsty-hames requested a review from guywillis June 19, 2023 12:00

StuartNicholls reviewed Jun 20, 2023

View reviewed changes

StuartNicholls approved these changes Jun 20, 2023

View reviewed changes

kirsty-hames merged commit bf2f2e6 into master Jul 4, 2023

kirsty-hames deleted the issue/443 branch July 4, 2023 15:38

github-actions bot added the released label Jul 4, 2023

kirsty-hames mentioned this pull request Jul 4, 2023

Replace hardcoded icon button border-radius with variable #451

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: replace notify button icon duplicate styles with mixin (fixes #443) #448

Update: replace notify button icon duplicate styles with mixin (fixes #443) #448

kirsty-hames commented Jun 15, 2023 •

edited

Loading

swashbuck left a comment

cahirodoherty-learningpool left a comment

StuartNicholls Jun 20, 2023

kirsty-hames Jun 20, 2023

StuartNicholls Jun 20, 2023

kirsty-hames Jul 4, 2023 •

edited

Loading

github-actions bot commented Jul 4, 2023

Update: replace notify button icon duplicate styles with mixin (fixes #443) #448

Update: replace notify button icon duplicate styles with mixin (fixes #443) #448

Conversation

kirsty-hames commented Jun 15, 2023 • edited Loading

swashbuck left a comment

Choose a reason for hiding this comment

cahirodoherty-learningpool left a comment

Choose a reason for hiding this comment

StuartNicholls Jun 20, 2023

Choose a reason for hiding this comment

kirsty-hames Jun 20, 2023

Choose a reason for hiding this comment

StuartNicholls Jun 20, 2023

Choose a reason for hiding this comment

kirsty-hames Jul 4, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jul 4, 2023

kirsty-hames commented Jun 15, 2023 •

edited

Loading

kirsty-hames Jul 4, 2023 •

edited

Loading