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: add generic .notify__btn-icon class (fixes #126) #127

Merged
merged 2 commits into from
Jul 4, 2023
Merged

Update: add generic .notify__btn-icon class (fixes #126) #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27eae15
add generic .notify__btn-icon class
kirsty-hames May 24, 2023
f8bb45f
remove duplicate LESS
kirsty-hames May 24, 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
15 changes: 0 additions & 15 deletions less/hotgridPopup.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,21 +133,6 @@
padding-top: (@icon-size + (@icon-padding * 2)) + (@item-padding / 2);
}

.btn-icon&__controls,
.btn-icon&__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;);
}
}

&__item-title {
margin-bottom: @notify-title-margin;
.notify-title;
Expand Down
6 changes: 3 additions & 3 deletions templates/hotgridPopupToolbar.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export default function HotgridPopupToolbar(props) {

<button
className={classes([
'btn-icon hotgrid-popup__controls back',
'btn-icon notify__btn-icon hotgrid-popup__controls back',
!shouldEnableBack && 'is-disabled'
])}
aria-label={backLabel}
Expand All @@ -52,7 +52,7 @@ export default function HotgridPopupToolbar(props) {

<button
className={classes([
'btn-icon hotgrid-popup__controls next',
'btn-icon notify__btn-icon hotgrid-popup__controls next',
!shouldEnableNext && 'is-disabled'
])}
aria-label={nextLabel}
Expand All @@ -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"
StuartNicholls marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

See here:

adaptlearning/adapt-contrib-vanilla#448 (comment)

Copy link

@StuartNicholls StuartNicholls Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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!

Copy link
Contributor Author

@kirsty-hames kirsty-hames Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

aria-label={ariaLabels.closePopup}
onClick={onCloseClick}
>
Expand Down