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

Fix: Fix setFocus() when using _isMobileTextBelowImage (fixes #271) #272

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

swashbuck
Copy link
Contributor

Fixes #271

Fix

  • Fix the animatedElement target in setFocus() when using _isMobileTextBelowImage. The element .narrative__strapline-header-inner doesn't exist when using _isMobileTextBelowImage: true which is why it was breaking

Testing

  1. Enable _isMobileTextBelowImage in a Narrative component
  2. Test on mobile

@swashbuck swashbuck marked this pull request as ready for review July 10, 2023 19:17
@swashbuck swashbuck self-assigned this Jul 10, 2023
@swashbuck swashbuck added the bug label Jul 10, 2023
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Contributor

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @swashbuck. Testing this, focusOnNarrativeElement needs updating too. Replacing lines 58 - 60 with the following resolves the issue for me. Testing on Mac Safari running VoiceOver, Chrome and iPhone.

    const $elementToFocus = this.isLargeMode() ||
    this.model.get('_isMobileTextBelowImage')
      ? this.$(`.narrative__content-item${dataIndexAttr}`)
      : this.$(`.narrative__strapline-btn${dataIndexAttr}`);

The focus and VoiceOver reading is consistent with the _isTextBelowImage experience.

@swashbuck
Copy link
Contributor Author

Testing this, focusOnNarrativeElement needs updating too.

@kirsty-hames Nice catch. This has been updated.

@swashbuck swashbuck requested a review from kirsty-hames July 11, 2023 14:19
@swashbuck swashbuck merged commit ab28273 into master Jul 11, 2023
@swashbuck swashbuck deleted the issue/271 branch July 11, 2023 16:22
@github-actions
Copy link

🎉 This PR is included in version 7.4.10 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_isMobileTextBelowImage back button issue
4 participants