Update: add generic .notify__btn-icon class (fixes #126) #127
Conversation
as these now inherit from .notify__btn-icon in Vanilla notify.less
|
PR might need tweaking pending Vanilla decision adaptlearning/adapt-contrib-vanilla#443 (comment) |
|
I've just updated the Vanilla PR to replace the Notify icon button styles with a mixin. For this PR, I'm inclined to keep the current implementation as the addition of the The alternative, to include the |
| @@ -67,7 +67,7 @@ export default function HotgridPopupToolbar(props) { | |||
| } | |||
|
|
|||
| <button | |||
| className="btn-icon hotgrid-popup__close" | |||
| className="btn-icon notify__btn-icon hotgrid-popup__close" | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
@kirsty-hames, currently we have notify__btn-icon next & notify__btn-icon back So, for consistency and to assist with this perhaps add close here?
That way we can still use the same class/selector and can target the different button types separately.
Note, I think the class naming is off here and should perhaps be is-next / is-previous & is-close. Another reason to rationalise these button classes further!
There was a problem hiding this comment.
For the purpose of separating the styles for controls and close buttons we could just do this targeting the existing selectors? See below
&__controls {
.notify-btn-icon;
}
&__close {
.notify-btn-icon;
}
I do agree the class naming is off here and I'm in favour of a shared selector e.g. .notify__btn-icon and the tag on button type selectors e.g. is-next etc but this will need addressing across Adapt for consistency so perhaps it's best to address this as part of the Adapt button issue. As in capture all button types, agree terminology so we don't have to keep changing templates and then roll out. Might be easier to keep track of consistency that way too.
There was a problem hiding this comment.
I was thinking that stay on target with notify__btn-icon. so not update next/back and just use:
&__btn-icon next, &__btn-icon back {
.notify-btn-icon;
}
&__btn-icon close {
.notify-btn-icon;
}
then when it comes to addressing the buttons in that issue I think perhaps this is a longer term problem that has many complexities - but perhaps we'd have adapt utility classes like 'primary-button' & 'secondary-button' (or whatever so they'd be called) in addition to the BEM naming already there. So the is- notation is sort of a separate issue, was just pointing out the problems!
I maybe getting confused here but aren't there only a small handful of places where this would need to change?
There was a problem hiding this comment.
I maybe getting confused here but aren't there only a small handful of places where this would need to change?
This applies to the following plugins/templates:
Controls: Hotgrid and Hotgraphic
Close: Notify popup, Notify push, Hotgraphic, Hotgrid and Tutor.
The existing PR satisfies the issue raised, to reduce duplicate code.
Based on suggestions above, I think next steps is to confirm the class naming for Adapt buttons and then roll out to plugins rather than tackling in this PR.
|
🎉 This PR is included in version 4.4.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Fixes #126
Apply
.notify__btn-iconto popup icon buttons to inherit styles from Vanilla notify.less.Remove the duplicate styles from Hotgrid (these just repeat what's now inherited from Vanilla).
This means the default Notify button icon 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.
Ref: similar PR raised for Hotgraphic adaptlearning/adapt-contrib-hotgraphic#276