Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jul 4, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
09a46f2
create mixin for notify icon button styles
kirsty-hames Jun 15, 2023
484fd1a
replace notifyPush close button styles with mixin
kirsty-hames Jun 15, 2023
5d0bb32
replace hotgraphicPopup icon button styles with mixin
kirsty-hames Jun 15, 2023
9bc0031
replace tutor close button styles with mixin
kirsty-hames Jun 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions less/core/notify.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,7 @@
}

&__btn-icon {
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;
}

&__close-btn {
Expand All @@ -109,3 +99,19 @@
// }
// }
}

// Global Notify button icon
// --------------------------------------------------
.notify-btn-icon() {
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;);
}
}
12 changes: 1 addition & 11 deletions less/core/notifyPush.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,7 @@
position: absolute;
top: 0;
right: 0;
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;

.dir-rtl & {
right: auto;
Expand Down
12 changes: 1 addition & 11 deletions less/plugins/adapt-contrib-hotgraphic/hotgraphicPopup.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,7 @@

&__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;
}
Comment on lines 9 to 12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kirsty-hames kirsty-hames Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .


&__item-title {
Expand Down
12 changes: 1 addition & 11 deletions less/plugins/adapt-contrib-tutor/tutor.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,7 @@
}

&__btn-icon {
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;
}

// overlay
Expand Down