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

New: Add option to make entire menu items clickable (fixes #157) #188

Closed
wants to merge 17 commits into from
Closed

New: Add option to make entire menu items clickable (fixes #157) #188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e794c5b
Add _areEntireItemsClickable option, update documentation
swashbuck Apr 9, 2024
732e195
Update schemas
swashbuck Apr 9, 2024
426b8c1
Optimizations to className() and events()
swashbuck Apr 9, 2024
372f4e1
Refactor className() using an array
swashbuck Apr 9, 2024
0dee1c9
Update attributes() to simplify getting parent attributes
swashbuck Apr 9, 2024
a4fa56d
Move click event to boxmenu-item__inner instead of boxmenu-item
swashbuck Apr 9, 2024
2c85651
Make areEntireItemsClickable a getter function
swashbuck Apr 9, 2024
8410ee8
Refactor attributes()
swashbuck Apr 9, 2024
7ccc459
Update itemCount in attributes()
swashbuck Apr 9, 2024
be16139
Move aria labels to .boxmenu-item__inner
swashbuck Apr 9, 2024
ad3f8c4
Move aria-label to new element
swashbuck Apr 18, 2024
01bcaee
Fix typo
swashbuck Apr 18, 2024
230d46a
Fix missing closing if
swashbuck Apr 18, 2024
f806326
Update README.md
oliverfoster Apr 19, 2024
df96e26
Make boxmenu-item__inner focusable by adding tabindex attribute
swashbuck Apr 19, 2024
0310042
Add keydown event for keyboard navigation
swashbuck May 9, 2024
14979b3
Move preventDefault
swashbuck May 14, 2024
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
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ The first value is the horizontal position and the second value is the vertical.

>>>**\_small** (number): The minimum height should only be used in instances where the menu header height needs to be greater than the content e.g. to prevent a background image being cropped.

>>>**\_areEntireItemsClickable** (boolean): Indicates if the entire menu item can be clicked to go to the target page. If `true`, the View button will be hidden.
Copy link
Member

Choose a reason for hiding this comment

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

I really don't like this setting name. I have feelsies it should be something like "_useTilesNotButtons" or "_useTiles" but I can't quite put my finger on it. I don't suppose it matters that much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not committed to this setting name. Could even be as simple as _hideItemButtons or _hasItemButtons.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've used _boxMenuItemIsClickable when doing similar on projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kirsty-hames @oliverfoster I like _boxMenuItemIsClickable. Shall I change it to that?

Copy link
Member

Choose a reason for hiding this comment

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

No other property in box menu is prefixed with _boxMenu.

Summary:
The behaviour hides the button and makes the item clickable. It is a layout alteration. The proposed names so far only describe either removing the button or making the item clickable, not both, which is more important? Is there another concept that encapsulates both changes? We haven't provided a rationale as to why one would enable/disable this property and choose the layout change.

Suggestion:
_isItemClickable perhaps? To keep it simple.


**\_globals** (object): The Globals object that contains value for **\_menu**.

>**\_menu** (object): The menu object that contains value for **\_boxMenu**.
Expand Down
1 change: 1 addition & 0 deletions example.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// course.json
"_boxMenu": {
"_areEntireItemsClickable": false,
"_graphic": {
"alt": "",
"_src": "course/en/images/logo-graphic.jpg"
Expand Down
17 changes: 14 additions & 3 deletions js/BoxMenuItemView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,34 @@
import MenuItemView from 'core/js/views/menuItemView';
import Adapt from 'core/js/adapt';
import router from 'core/js/router';

class BoxMenuItemView extends MenuItemView {

className() {
return `${super.className()} boxmenu-item`;
return [
super.className(),
'boxmenu-item',
this.areEntireItemsClickable && 'is-entire-item-clickable'
].filter(Boolean).join(' ');
}

events() {
return {
'click .js-btn-click': 'onClickMenuItemButton'
'click .js-btn-click': 'onClickMenuItemButton',
'keydown .js-btn-click': 'onClickMenuItemButton'
};
}

onClickMenuItemButton(event) {
if (event && event.preventDefault) event.preventDefault();
if (event.code && !['Space', 'Enter'].includes(event.code)) return;
if (event?.preventDefault) event.preventDefault();
if (this.model.get('_isLocked')) return;
router.navigateToElement(this.model.get('_id'));
}

get areEntireItemsClickable() {
return Adapt.course.get('_boxMenu')?._areEntireItemsClickable;
oliverfoster marked this conversation as resolved.
Show resolved Hide resolved
}
}

BoxMenuItemView.template = 'boxMenuItem';
Expand Down
4 changes: 4 additions & 0 deletions less/boxMenuItem.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,8 @@
@media (min-width: @device-width-medium) {
width: 50%;
}

&.is-entire-item-clickable:not(.is-locked) {
cursor: pointer;
}
}
9 changes: 9 additions & 0 deletions properties.schema
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@
"type": "object",
"required": false,
"properties": {
"_areEntireItemsClickable": {
"type": "boolean",
"required": false,
"default": false,
"title": "Are entire menu items clickable?",
"inputType": "Checkbox",
"validators": [],
"help": "Enable this option to make each menu item clickable. The menu item button will be hidden."
},
"_graphic": {
"type": "object",
"required": false,
Expand Down
6 changes: 6 additions & 0 deletions schema/course.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@
"title": "Box Menu",
"default": {},
"properties": {
"_areEntireItemsClickable": {
"type": "boolean",
"title": "Are entire menu items clickable?",
"description": "Enable this option to make each menu item clickable. The menu item button will be hidden.",
"default": false
},
"_graphic": {
"type": "object",
"title": "Menu logo image",
Expand Down
10 changes: 9 additions & 1 deletion templates/boxMenuItem.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
{{import_globals}}
{{import_adapt}}

<div class="menu-item__inner boxmenu-item__inner">
<div class="menu-item__inner boxmenu-item__inner{{#if Adapt.course._boxMenu._areEntireItemsClickable}} js-btn-click{{/if}}"{{#if Adapt.course._boxMenu._areEntireItemsClickable}} role="link" tabindex="0"{{/if}}{{#if _isLocked}} aria-disabled="true"{{/if}}>
{{#if Adapt.course._boxMenu._areEntireItemsClickable}}
<span class="aria-label">
{{#if _isComplete}}{{_globals._accessibility._ariaLabels.complete}}. {{else _isVisited}}{{_globals._accessibility._ariaLabels.visited}}. {{/if}}{{#if _isLocked}}{{_globals._accessibility._ariaLabels.locked}}. {{else}}{{linkText}} {{/if}}{{title}}. {{{compile _globals._menu._boxMenu.itemCount}}}.{{#if _isOptional}} {{_globals._accessibility._ariaLabels.optional}}{{/if}}
</span>
{{/if}}

{{#if _graphic.src}}
<div class="menu-item__image-container boxmenu-item__image-container">
Expand Down Expand Up @@ -40,9 +46,11 @@
</div>

<div class="menu-item__button-container boxmenu-item__button-container">
{{#unless Adapt.course._boxMenu._areEntireItemsClickable}}
oliverfoster marked this conversation as resolved.
Show resolved Hide resolved
<button class="btn-text menu-item__button boxmenu-item__button js-btn-click{{#if _isVisited}} is-visited{{/if}}{{#if _isLocked}} is-locked{{/if}}" aria-label="{{#if _isComplete}}{{_globals._accessibility._ariaLabels.complete}}. {{else _isVisited}}{{_globals._accessibility._ariaLabels.visited}}. {{/if}}{{#if _isLocked}}{{_globals._accessibility._ariaLabels.locked}}. {{else}}{{linkText}} {{/if}}{{title}}. {{{compile _globals._menu._boxMenu.itemCount}}}.{{#if _isOptional}} {{_globals._accessibility._ariaLabels.optional}}{{/if}}"{{#if _isLocked}} aria-disabled="true"{{/if}} role="link">
{{{linkText}}}
</button>
{{/unless}}

<span class='menu-item__status boxmenu-item__status'>
<span class='icon' aria-hidden="true"></span>
Expand Down