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

_canCycleThroughPagination conflict with narrative controls #286

Closed
chrisgillison opened this issue Oct 24, 2023 · 7 comments · Fixed by #303
Closed

_canCycleThroughPagination conflict with narrative controls #286

chrisgillison opened this issue Oct 24, 2023 · 7 comments · Fixed by #303

Comments

@chrisgillison
Copy link

Subject of the issue/enhancement/features

I think this is a bug. Certainly not intuitive.

Your environment

FW 5.33.1
The hotgraphic version I'm using is 6.7.0, but I've just created a brand new course and the same issue exists in hotgraphic version 6.8.0

Steps to reproduce

In your hotgraphic component, set "_canCycleThroughPagination": true; and "_isNarrativeOnMobile": true;

Expected behaviour

When the HG reverts to narrative on mobile, the right narrative control should either be disabled when the last item is reached, or it should be enabled and take you back to the first item (i.e. cycle).

Actual behaviour

It's not doing either - it's enabled but doesn't do anything.

@joe-allen-89 joe-allen-89 self-assigned this Oct 30, 2023
@joe-allen-89
Copy link
Contributor

Currently the narrative controls asses if they should be enabled with canCycleThroughPagination but are unable to cycle like the hotgraphic.

To fix this issue we have 2 options,

  • update the narrative to cycle like the hotgraphic does, or,
  • remove canCycleThroughPagination when assessing if controls should be enabled.

Any preferences to this? Any potential downsides or ramifications?

@oliverfoster
Copy link
Member

  • remove canCycleThroughPagination when assessing if controls should be enabled.

Do you have a code reference? This seems a bit weird.

I'm not sure if cycling the narrative is a good idea. It's easy to know when you're at the end because you can't go any further. There is little indication that you're complete otherwise.

@joe-allen-89
Copy link
Contributor

@oliverfoster -

const shouldEnableNext = index < totalItems - 1 || canCycleThroughPagination;

Good point on indicating completion.

@StuartNicholls
Copy link

I'm also in favour of removing the canCycleThroughPagination check. 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 and as Ollie says, its easy to know when you're done.

Having said that, I'd argue narrative does need a visual representation of completeness on the component I think. But that's a separate issue.

@oliverfoster
Copy link
Member

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.

I think this is slightly different. Pagination as a whole, displaying the back and next buttons is the bit that alleviates the cumbersome nature of having to click, close, click, close the pins.

You can see the two properties and their descriptions here https://github.com/adaptlearning/adapt-contrib-hotgraphic/blob/0a9504acee1229e9eee9f443312cce6fbf7d04c9/properties.schema#L140-L157

const shouldEnableNext = index < totalItems - 1 || canCycleThroughPagination;

canCycleThroughPagination seems intentful enough as a flag for looping around at the ends. Putting it more into context and looking at the code, I see no reason why we shouldn't allow cycling the pagination in narrative. It may give us more impetus to fix the completion indication issue.

@StuartNicholls
Copy link

I think this is slightly different. Pagination as a whole, displaying the back and next buttons is the bit that alleviates the cumbersome nature of having to click, close, click, close the pins.

Yep, that's what I was getting at.

canCycleThroughPagination seems intentful enough as a flag for looping around at the ends. Putting it more into context and looking at the code, I see no reason why we shouldn't allow cycling the pagination in narrative. It may give us more impetus to fix the completion indication issue.

Sure, I wouldn't say that it shouldn't have it. I was really agreeing as my thoughts were its an unnecessary config option in that I don't recall it ever been requested. Happy to go with the consensus however.

@joe-allen-89 joe-allen-89 transferred this issue from adaptlearning/adapt-contrib-hotgraphic Dec 8, 2023
@joe-allen-89 joe-allen-89 moved this from Assigned to Needs Reviewing in adapt_framework: The TODO Board Dec 8, 2023
@joe-allen-89 joe-allen-89 moved this from Needs Reviewing to Assigned in adapt_framework: The TODO Board Feb 19, 2024
@joe-allen-89 joe-allen-89 moved this from Assigned to Needs Reviewing in adapt_framework: The TODO Board Jun 4, 2024
kirsty-hames pushed a commit that referenced this issue Jun 11, 2024
* Fix: remove check for canCycleThroughPagination

* canCycleThroughPagination check removed from setupBackNextLabels
@github-project-automation github-project-automation bot moved this from Needs Reviewing to Recently Released in adapt_framework: The TODO Board Jun 11, 2024
github-actions bot pushed a commit that referenced this issue Jun 11, 2024
## [7.9.1](v7.9.0...v7.9.1) (2024-06-11)

### Fix

* remove check for canCycleThroughPagination (fixes #286) (#303) ([d16bd53](d16bd53)), closes [#286](#286) [#303](#303)
Copy link

🎉 This issue has been resolved in version 7.9.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment