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.

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

Configurable option to make entire menu item clickable #157

Closed
swashbuck opened this issue Jun 27, 2023 · 14 comments
Closed

Configurable option to make entire menu item clickable #157

swashbuck opened this issue Jun 27, 2023 · 14 comments

Comments

@swashbuck
Copy link
Contributor

swashbuck commented Jun 27, 2023

Subject of the enhancement

Currently, clicking the "View" button is the only way to view the page for a menu item. We commonly modify this so that the entire menu item can be clicked instead of just the "View" button. Ideally, we would have a configurable option for this.

We should also explore if there are any accessibility or other considerations.

menu-item-clickable

@joe-replin
Copy link
Contributor

joe-replin commented Jun 27, 2023

We see a need for this in projects, but I also feel that doing this negates the purpose of the 'View' button and therefore should be removed. For screen-readers & tabbing, menu-item needs to become its own < button >, and then it needs to take on aria & model status information such as 'locked' & 'completed'. Which complicates things when a project calls for no 'View' button.

@guywillis
Copy link
Contributor

I appreciate the desire for this functionality but I think we should assess from a user experience perspective whether we should be making this change.

  • Is it intuitive that the menu item is clickable without a visible button?
  • Would there be a desire / requirement to also change the visuals of the menu items alongside the functional change?
  • Would an alternative 'tile' menu plugin approach be better or worse?
  • Would adding this functionality increase / decrease / negligibly impact maintenance overhead in the long term?

@swashbuck
Copy link
Contributor Author

swashbuck commented Jun 28, 2023

@guywillis Thanks for the feedback on this.

Is it intuitive that the menu item is clickable without a visible button?

My thoughts:

  • Modern audiences are a lot more sophisticated these days. Having an explicit "click here" button isn't necessary.
  • Any usability concerns can be mitigated via the theme. For instance, adding a “view” icon on hover or emphasizing focus outlines for users navigating via keyboard.
  • We wouldn't be forcing this behavior by default. Devs and art directors can keep the "View" button if that is their preference.

Some examples of modern web sites where there is no button to indicate that items are clickable:

Netflix

The Guardian

Twitter



Would there be a desire / requirement to also change the visuals of the menu items alongside the functional change?

No, this would not affect the visuals. However, we could add two new options. _isEntireMenuItemClickable to make the menu item clickable. Then, _isViewButtonDisabled to remove the View button.

Aside from configuration, we may want to make some structural changes as well. For instance, when the View button is hidden, we might want to do the following:

  • The .menu-item could switch to either a <button> element or have a role="button" applied.
  • The .menu-item would use an aria-label that contains the item's title, body, duration, and progress information

Would an alternative 'tile' menu plugin approach be better or worse?

A 'tile' menu would be nice to have, maybe something like the Hot Graphic's grid layout? However, this adds a completely new menu plugin that we would need to maintain. Also, I see a 'tile' menu as having a distinct look and feel compared to the Box Menu.

Would adding this functionality increase / decrease / negligibly impact maintenance overhead in the long term?

As with any new feature, the level of future maintenance required would slightly increase. However, I don't feel that it would be an excessive burden as the changes wouldn't be too extreme.

Overall, my main concern is that this functionality requires changes to the plugin’s JavaScript. This is a barrier when avoiding core plugin changes (as we do with the AAT) and trying to stay up to date with the latest releases.

@guywillis
Copy link
Contributor

Lovely response @swashbuck, thank you.

  • Modern audiences are a lot more sophisticated these days. Having an explicit "click here" button isn't necessary.
  • Any usability concerns can be mitigated via the theme. For instance, adding a “view” icon on hover or emphasizing focus outlines for users navigating via keyboard.
  • We wouldn't be forcing this behavior by default. Devs and art directors can keep the "View" button if that is their preference.

Some examples of modern web sites where there is no button to indicate that items are clickable:

No, this would not affect the visuals. However, we could add two new options. _isEntireMenuItemClickable to make the menu item clickable. Then, _isViewButtonDisabled to remove the View button.

Agreed that audiences are more aware of general practices when it comes to the web.

It's interesting that in each of the example the amount of emphasis the image gets. Notably, in the Guardian and Netflix examples there are visual aides to indicate behaviour on hover like the background colour changing, the text being underlined, and the image zoom / tile dimension increase. The Twitter example bucks the trend but not having any hover elements but, in my opinion, doesn't necessarily need them given the implied functionality of clicking an image to enlarge.

Perhaps we can take some these, or other, approaches forward to ensure there is a visual jump off for users.

I'm currently on the fence with having two configuration options _isEntireMenuItemClickable and _isViewButtonDisabled. To me, they imply the same, or very similar, functionality. It might also be easier to accommodate the added functionality by narrowing down the scope.

Overall though, I do welcome the change and think it would enhance the plugins offering.

@oliverfoster
Copy link
Member

It feels like boxMenu works well as a generic menu because it can be modified rather easily into other menu styles.

I think _isEntireMenuItemClickable and _isViewButtonDisabled are effectively identical, you'd not want a button and the background to be clickable and you wouldn't want no button and no background click.

How would locking look in the absence of a button icon?

What would the menu you're thinking of generally look like? @swashbuck

Is it just boxMenu with the items as button?
image

Or width wise?
image

Perhaps a few designs would be good?

@StuartNicholls
Copy link
Contributor

StuartNicholls commented Jul 21, 2023

Hey @swashbuck. I've been working on a menu similar to box menu and have had to grapple with this issue already.

To echo what @oliverfoster says, I don't think we need a switch for the button if there's a switch to change the link. Although we have had a clickable 'box' with the button still present in the past, I don't think this is great UX/UI anyway. The way I've handled it - although not sure if this is a good idea - is, if there was no text set for the 'view' button, then the click would automatically get attached to the 'box' getting rid of the need for extra config. So the default is click on the box, unless you add text to 'linkText' config (and be descriptive in AAT) - not sure if this is a good idea or not!

But - to also echo @oliverfoster - this then adds complications with locking, and in addition, completion status (check mark)- can't remember if this has been added to boxmenu yet? The button area is an ideal location for both of these. But once the button isn't there, it becomes less clear where these would go.

And again, exactly what Ollie has already said, what does the 'box' look like? Does it remain like it is, or do we then want to add background images to box menu items, so they look more like your Netflix example etc.?

My personal feeling is adding a switch to boxmenu might add too much complexity (and open a can of worms) and it might be a better idea to just choose one approach and leave it at that. It might be that having the whole item clickable is the better way forward and we can now lose the button. I think if I had raised the issue thats what I'd be suggesting having investigated this thoroughly for the new menu.

@swashbuck swashbuck moved this from Assigned to Awaiting Response in adapt_framework: The TODO Board Aug 23, 2023
@swashbuck
Copy link
Contributor Author

swashbuck commented Aug 23, 2023

I think _isEntireMenuItemClickable and _isViewButtonDisabled are effectively identical, you'd not want a button and the background to be clickable and you wouldn't want no button and no background click.

@oliverfoster Yep, this makes sense. I would be fine with only one option.

it might be a better idea to just choose one approach and leave it at that.

@StuartNicholls This could work, too. My preference, though, would be to keep the button as an option for the edge cases where the user base is primarily older or less tech-savvy users. Or if the art director just really wants it as a design element.

So the default is click on the box, unless you add text to 'linkText' config (and be descriptive in AAT) - not sure if this is a good idea or not!

@StuartNicholls I do like this approach as long as we think it would be clear to the people building the courses. In the help popup for the button text (linkText), we could say "leave blank to make the entire menu item clickable".

we have had a clickable 'box' with the button still present in the past, I don't think this is great UX/UI anyway.

@StuartNicholls Yes, agree, this is probably confusing to the user especially when using keyboard navigation. Only one element should be clickable.

Perhaps a few designs would be good?

My intention with this ticket was to only look at the JavaScript functionality since that cannot be changed via a custom theme (via CSS styles or templates). The templates may need adjusting as well (ex. updating aria-labels). Iterating on different layouts may be out of scope and better suited for another ticket like adaptlearning/adapt-contrib-vanilla#466?

That said, here are my initial thoughts on what this would look like.

First, the current view:
Current view

Next, this is what we'd see if we simply removed the button without any other changes:
No buttons

Finally, this is my preference for when the button is removed:
No buttons, alt layout

Beyond this, as I said, layout changes can be made via custom theme. It's mainly the click functionality that requires changes to the menu plugin itself.

@swashbuck
Copy link
Contributor Author

swashbuck commented Mar 29, 2024

Hi, all. Circling back to this ticket, I would propose that we limit the scope to the following changes:

  1. When linkText is empty, change the element type of .menu-item to <button>. When it is a button, the .menu-item will need to contain the same aria-label as the current "View" button.
  2. When linkText is empty, hide the "View" button (.menu-item__button).
  3. Change all div elements to span elements. This is required when we make .menu-item a <button> since it can only contain phrasing content. These span elements will then need to use display: block or display: flex.

Visual changes (aside from hiding the current "View" button) could be handled separately in another ticket.

Please let me know your thoughts on moving forward with a PR.

Edit: Alternatively, I'm wondering if we can just add role="button" to the .menu-item div. This would eliminate the need for the third step, as well as saving us from having to style the .menu-item as a <button> element.

@guywillis
Copy link
Contributor

guywillis commented Apr 2, 2024

I'm a little hesitant to approve the scoped changes given the required numbers of alterations to the template file to adopt the solution.

As much as I agree that it would be a nice addition to the plugin my fear is that this change leads to code bloat.

Perhaps the cleanest solution is to simply have a different menu plugin that addresses these issues?

@swashbuck
Copy link
Contributor Author

@guywillis All good points. I'm just not sure if there's more tech debt associated with an entirely new plugin vs. adding this feature into the current box menu.

One additional note is that my third point above would apply in all cases rather than in one or the other scenario.

Perhaps we could discuss this in an upcoming dev meeting?

@guywillis
Copy link
Contributor

@guywillis All good points. I'm just not sure if there's more tech debt associated with an entirely new plugin vs. adding this feature into the current box menu.

One additional note is that my third point above would apply in all cases rather than in one or the other scenario.

Perhaps we could discuss this in an upcoming dev meeting?

It may well add more technical debt to have a new plugin but the offset is that the boxmenu remains a 'clean' base to build on top of. Perhaps there are additional features that could be added to the new plugin that can't be added into the boxMenu due to certain restrictions?

I'm curious as to why the change to <span> through out is a required change in all cases when it's not required currently - is this something that we should be adopting at present outside of this change?

Very happy to discuss to move the issue forward.

@swashbuck
Copy link
Contributor Author

I'm curious as to why the change to through out is a required change in all cases when it's not required currently - is this something that we should be adopting at present outside of this change?

@guywillis From my understanding, <button> elements cannot contain <div> elements but they can contain <span> elements. However, I think adding role="button" to the .menu-item div would be sufficient. So, we wouldn't necessarily have to change that to an actual <button> element as I'd initially thought.

It may well add more technical debt to have a new plugin but the offset is that the boxmenu remains a 'clean' base to build on top of. Perhaps there are additional features that could be added to the new plugin that can't be added into the boxMenu due to certain restrictions?

Yep, that makes sense. I'm fine to go either route depending on what the consensus is.

By the way, this requirement came up again recently, which is why I restarted the conversation. I have a workaround for our project, though, using a separate extension that modifies the Box Menu behavior.

@oliverfoster
Copy link
Member

Can you do a pr @swashbuck?

Then we can all have a better look at it and decide to approve or not. I think that will resolve the impasse.

@swashbuck
Copy link
Contributor Author

Closing this issue (see #188 for details). In favor of exploring as a new plugin.

@swashbuck swashbuck moved this from Assigned to Needs Reviewing in adapt_framework: The TODO Board Apr 19, 2024
@swashbuck swashbuck moved this from Needs Reviewing to Assigned in adapt_framework: The TODO Board Jun 10, 2024
@swashbuck swashbuck closed this as not planned Won't fix, can't repro, duplicate, stale Jan 14, 2025
@github-project-automation github-project-automation bot moved this from Backlog to Recently Released in adapt_framework: The TODO Board Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Recently Released
Development

Successfully merging a pull request may close this issue.

5 participants