adaptlearning / adapt-contrib-narrative

A component that displays an image gallery with accompanying text
GNU General Public License v3.0
5 stars 39 forks source link

New: _canCycleThroughPagination functionality added (fixes #286) #287

Closed joe-allen-89 closed 1 month ago

joe-allen-89 commented 7 months ago

286

New

Potentially put this release on hold until we can discuss/come up with a better way of indicating completion.

kirsty-hames commented 6 months ago

Potentially put this release on hold until we can discuss/come up with a better way of indicating completion.

Hey @joe-allen-89, I've added some suggestions regarding the display of completion in https://github.com/adaptlearning/adapt-contrib-narrative/issues/288

kirsty-hames commented 6 months ago

Functionality wise, this works as expected testing in Chrome/Safari/FF on Mac and Safari iPhone.

When tabbing/testing with a screen reader, because the back button receives focus before the next button, the back button is read as "Back to Narrative item 3 title (item 3 of 3)". I think this is confusing when you haven't visited the first item yet. From a UX perspective, I'd expect a Narrative to be a linear 1, 2, 3 etc. With this in mind, I think I'm inclined to agree with some of the comments made in the original issue.

Narrative is functionally differrent to the hotgraphic. The main reason cycling is requested for hotgraphic is because you can 'start' at any pin and clients find it too cumbersome to click in and out of pin/notify. Narrative doesn't have this issue as it always starts at the beginning.

oliverfoster commented 5 months ago

This issue is only really to solve what happens with the _canCycleThroughPagination property when a narrative replaces a hotgraphic on mobile, where the hotgraphic has _canCycleThroughPagination=true.

Narrative did not have its own _canCycleThroughPagination property. Narrative is always linear, it always starts from index 0 (akin to an ordered list). Hotgraphic is not linear, the pins can be clicked in any order (akin to an unordered list). When the narrative replaces the hotgraphic, the list that it represents does not inherit extra order that looping/cycling would break.

In the exploration of this issue we have identified that the narrative does not show completion #288.

Currently, when the narrative replaces a hotgraphic with _canCycleThroughPagination=true, the narrative buttons are not disabled at the ends but they also do not loop/cycle. The buttons are enabled but do nothing #286.

We have two possible choices to solve this issue:

  1. Always disable the narrative buttons at the ends
  2. Allow the narrative buttons to loop/cycle when the narrative replaces a hotgraphic on mobile where the hotgraphic has _canCycleThroughPagination=true

When tabbing/testing with a screen reader, because the back button receives focus before the next button, the back button is read as "Back to Narrative item 3 title (item 3 of 3)".

This is as expected with a looping carousel/narrative according to wcag.

Are you in favour of always disabling the narrative buttons at the ends? @kirsty-hames

I'd like to add _canCycleThroughPagination to the narrative, with your completion fixes and possibly re-add the ability to click the indicators as it is in the wcag implementation.

kirsty-hames commented 5 months ago

Thanks for capturing all the discussion points @oliverfoster.

When tabbing/testing with a screen reader, because the back button receives focus before the next button, the back button is read as "Back to Narrative item 3 title (item 3 of 3)".

This is as expected with a looping carousel/narrative according to wcag.

Are you in favour of always disabling the narrative buttons at the ends? @kirsty-hames

I personally find it odd and would be in favour of always disabling the narrative buttons at the ends. However, regardless of personal opinion, if wcag have set a standard users will be expecting this functionality if it exists elsewhere.

I'd like to add _canCycleThroughPagination to the narrative, with your https://github.com/adaptlearning/adapt-contrib-narrative/issues/288 and possibly re-add the ability to click the indicators as it is in the wcag implementation.

Same goes for re-adding the ability to click the indicators. I personally find the wcag implementation cumbersome to navigate and read the info of each slide. That could be down to my SR keyboard skills though. I'm less keen on adding this but again happy to follow wcag for setting consistent functionality/UI.

Where there can be a lot of difference in opinion when it comes to implementation and this is often questioned by clients. Following wcag does help with those conversations and we have a reference point for following best practice.

oliverfoster commented 5 months ago

Following wcag does help with those conversations and we have a reference point for following best practice.

We can choose not to follow.

I agree that the clickable indicators is an odd one. Having them clickable is handy and expected oftentimes for sighted users but terrible for screen reader users. If included it has to be included for both.

The looping behaviour is usually for unordered carousels, having items that could appear in any order. Narrative can also and usually does a linear story with order, where the looping behaviour is a bit less meaningful. Without the completion indication, looping on a mandatory narrative would be a bit unfair, the user would have to remember that they'd seen the first slide or notice the backward slide.

I think the ordered/unordered list distinction is where the contention between wcag and Adapt comes from.

joe-allen-89 commented 2 months ago

Moving this ticket back to needs reviewing as https://github.com/adaptlearning/adapt-contrib-narrative/pull/301 has been merged allowing us to show completion within the progress indicators.

guywillis commented 1 month ago

Functionally, the proposed works as you'd expect.

However, I agree with Kirsty about the read order being confusing to non visual users. I think I would be in favour of always disabling start and end narrative buttons regardless of the Hotgraphic setting.

joe-allen-89 commented 1 month ago

After discussion on the OS call most people agree with not including the cycling functionality to the narrative and instead disabling the back/next buttons on last/first items. Moving back to assigned for changes to be made.

joe-allen-89 commented 1 month ago

Closing PR, as discussed the functionality is changing and a new PR will be raised.