search

cgkineo / adapt-pageNav

Navigation bar component (quicknav clone)
GNU General Public License v3.0
6 stars 4 forks source link

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

Closed swashbuck closed 6 months ago

swashbuck commented 1 year ago

Fix #41 #48

Fix

Breaking

Dependencies

Tooltip issue fix: https://github.com/adaptlearning/adapt-contrib-core/pull/464

Testing

Ideally, please include the following in your testing:

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.

LTR

Right-to-left (RTL)

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

RTL
kirsty-hames commented 11 months ago

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. root_1 Go back into the page. Tooltip still displays. root_2 Select _next, you're navigated to the next page. Tooltip still displays. root_3

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

swashbuck commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

re tooltips not disappearing:

pageNav does this on button click: https://github.com/cgkineo/adapt-pageNav/blob/a380c85b1c16151be507f825eb7f2c3287c2a109/js/PageNavView.js#L81

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 commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

@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 commented 10 months ago

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

@oliverfoster Ok, I've created https://github.com/adaptlearning/adapt-contrib-core/issues/463 , so I'll take a look there.

swashbuck commented 10 months ago

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

oliverfoster commented 10 months ago

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

swashbuck commented 10 months ago

@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 commented 10 months ago

Sure. Sounds good.

swashbuck commented 8 months ago

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[bot] commented 6 months ago

:tada: This PR is included in version 3.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: