Since labels will be shown by default in the new version of Virtual Tour, we need to make sure this does not happen to existing content. I.e: we need to set the behaviour.label.showLabel = true
This is done using the “content-upgrade” feature, which is described here: https://h5p.org/documentation/developers/content-upgrade. There are many examples in our existing content types. E.g:
Content upgrade is only available when the library is minor-bumped. I.e: for this case, we need to bump it to 0.4.0. Then it will be possible to run the upgrade script through e.g. the Drupal admin interface.
I’ve just done this but will need someone to review as I don’t really understand what I’ve done and the is no way to test it!
Reading Pål’s description I’m not sure he is aware of what the ‘showLabel’ attribute does. It doesn’t disable the feature but instead makes it a ‘hover only’.
I’ve been thinking perhaps this attribute should be renamed to `hoverLabel` to prevent confusion like this. It has also resulted in confusing false returns that end being true - https://github.com/h5p/h5p-three-image/blob/label/scripts/components/Shared/NavigationButtonLabel.js#L19
Existing Virtual Tour’s will use their Title, Text or Alternative Text (one of which is a mandatory field for all activity types) as it’s label. This should mean there isn’t a label that is totally inappropriate.
The only design consideration is to do with new labels which expand outside of the view, but that should be dealt with by issue
I am also confused by the description I created
For you comment about “hover only”, I think we can live with existing content displaying the titles on hover. But it’s really up to .
I have added a few comments here:
Other than that:
Your commit messages are not following the guidelines: https://chris.beams.io/posts/git-commit/. Instead of “Updated upgrade.js“, a better message would e.g be “Set showLabel to false in content upgrade“.
Pst: I will do a demo of the content upgrade feature tomorrow, so that you know how to actually test this
I have had a look at the latest commit, and all of my feedback is addressed. Great