Breaking: Update and modernize, simplify configuration (fixes #41 #48)

[swashbuck]

Fix #41 #48

Fix

• Convert to ES6 syntax
• Convert templates to JSX
• Add missing _iconClass help text to properties.schema
• Adds default icon values to schemas so that they are pre-populated in the AAT
• Update README.md and example.json
• Integration with Tooltip API. Bumps required framework version to 5.30.2.

Breaking

• Replaces _alignIconRight with _iconAlignment. Suggest using 'auto' for all buttons which adjusts for RTL vs. LTR. The Next button icon will also be adjusted accordingly when using 'auto' (see examples below)
• Removes _sibling buttons
• Removes _returnToPreviousLocation button

Dependencies

Tooltip issue fix: adaptlearning/adapt-contrib-core#464

Testing

Ideally, please include the following in your testing:

• All five button types
• With icons only, with text only, and with both icons and text
• All icon alignment options
• LTR vs. RTL with respect to the icon alignment

Icon alignment

The following demonstrates the options for _iconAlignment using LTR vs. RTL.

Left-to-right (LTR)

Note that the Next button icon should be right-aligned when using auto.

Right-to-left (RTL)

Note that the Next button icon should be left-aligned when using auto.

[kirsty-hames]

Text and/or icon display is working as expected thanks @swashbuck. Testing all _iconAlignment options in both RTL/LTR.

I have discovered a tooltip display issue which seems to be specific to Safari. When selecting the _root or _up button, the tooltip continues to display. See screenshots below.

When selecting _root, you're navigated to the menu. Tooltip still displays.

Go back into the page. Tooltip still displays.

Select _next, you're navigated to the next page. Tooltip still displays.

I was able to replicate this on Safari Mac, iPad and iPhone. Android tablet worked as expected.

[swashbuck]

Good catch, @kirsty-hames . I can reproduce the issue, too. It also seems that it's sometimes adding whitespace to the bottom of the page:

Both issues seem to resolve when you resize the browser window.

@oliverfoster Do you think this is an issue with the Tooltip API itself?

[oliverfoster]

I don't know, it shouldn't, it's all absolutely / fixed position so shouldn't take up space in the document flow. Could you investigate please?

[oliverfoster]

re tooltips not disappearing:

pageNav does this on button click:

adapt-pageNav/js/PageNavView.js

Line 81 in a380c85

router.navigateToElement(id);

a tooltip should call remove when any [data-tooltip-id] element emits a blur event:
https://github.com/adaptlearning/adapt-contrib-core/blob/6be73dead91e631ec5eeb2cacf34f0a33a06837a/js/views/TooltipItemView.js#L133
https://github.com/adaptlearning/adapt-contrib-core/blob/6be73dead91e631ec5eeb2cacf34f0a33a06837a/js/views/TooltipItemView.js#L48C40-L48C57

It may be a lifecycle/timing issue between the button click, mouseover to show the tooltip, and the navigate. Tooltips may be displaying the tooltip after it should listen for blur events?

[swashbuck]

@oliverfoster In 47c42b1, I force the Page Nav button to fire a blur event by focusing on the body. Previously, it wasn't firing onMouseOut() in TooltipItemView.js after clicking it. This seems to fix the problem, but do you think it has any ramifications for accessibility and screen reader focus?

Otherwise, @kirsty-hames , is this now working for you?

[kirsty-hames]

@oliverfoster In 47c42b1, I force the Page Nav button to fire a blur event by focusing on the body. Previously, it wasn't firing onMouseOut() in TooltipItemView.js after clicking it. This seems to fix the problem, but do you think it has any ramifications for accessibility and screen reader focus?

Otherwise, @kirsty-hames , is this now working for you?

Thanks for looking into this @swashbuck. I can confirm this is working as expected in Safari and I gave this a quick run through with voiceOver. Just FYI, I may have found a global issue with tooltip focus (not sure if it's expected functionality) which I'll query as an issue on Core - see issue for ref.

[oliverfoster]

@oliverfoster In 47c42b1, I force the Page Nav button to fire a blur event by focusing on the body. Previously, it wasn't firing onMouseOut() in TooltipItemView.js after clicking it. This seems to fix the problem, but do you think it has any ramifications for accessibility and screen reader focus?

Otherwise, @kirsty-hames , is this now working for you?

I think it would be better if the tooltips hid themselves on navigating to a new page.

[swashbuck]

I think it would be better if the tooltips hid themselves on navigating to a new page.

@oliverfoster Ok, I've created adaptlearning/adapt-contrib-core#463 , so I'll take a look there.

[swashbuck]

@kirsty-hames @oliverfoster Will you review adaptlearning/adapt-contrib-core#464 ? I believe that will fix the tooltip issue.

[kirsty-hames]

I've retested this with Core PR 465 and can confirm adaptlearning/adapt-contrib-core#463 resolved.

[oliverfoster]

Are we happy that this will break existing AATs if installed over the top?

[swashbuck]

@danielghost @kineopete Just to clarify, was the decision to hold off on this PR until the migration scripts are ready? If so, I can add a "needs migration" label here.

[oliverfoster]

Sure. Sounds good.

[swashbuck]

I would like to make a case for merging this PR now rather than waiting for the migration scripts.

1. The only breaking change that the migration scripts would address would be the icon alignment. The _returnToPreviousLocation and _sibling options have simply been removed, so there's nothing to migrate.
2. I have set up the icon alignment with sensible defaults. For instance, the icon for the _previous button is positioned on the left whereas the icon for the _next button is on the right. Then, for RTL, it's the reverse.
3. If there is a case where the icon has been positioned on the top or bottom, that would have been handled via custom theme work anyway. So, there wouldn't be a migration option for that.

Happy to discuss further in our next team call.

[github-actions]

🎉 This PR is included in version 3.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀