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 / remove focus on start watching button due to inconsistency #180

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

MelissaDTH
Copy link

@MelissaDTH MelissaDTH commented Apr 17, 2024

Description

  • Removes setting the focus on the start watching button due to inconsistent behaviour || OTT-1300
    • The original solution to apply a blur when clicking on a Card to make sure the body element is focused, was also inconsistent. If you'd then start tabbing, the focus lands on the footer, suggesting it's still on the previous Card in the CardGrid.
    • It has been internally discussed with the team and other solutions have been tried, such as setting the focus on the body via a different route or setting it on the 'skip to content link', with no success. Therefore, it's decided to remove the focus (also the key prop on the StartWatching button) and accept that the focus stays on the previous card when navigating from the CardGrid.

Update 17th of April:

The modifications in this PR do not alter the current behavior in the app. Submitting this PR to JW that reverts recent changes made us is not necessary. Discussed with Roy and Chris to label this PR as a draft and further explore another solution proposed by Chris, see: Slack link.

Christiaan's idea:

  • Responds to history changes causing all pages to disappear.
  • Only scroll to top when navigating forward (back handles this itself).
  • Do nothing if the user modal is open (or closes) because it handles focus itself.
  • The target main is also accessible during loading.
const useFocusOnNavigate = () => {
  const location = useLocation();
  const lastStateIdRef = useRef(0);
  const prevLocation = useRef(location);

  useEffect(() => {
    const stateId = window.history.state?.idx;
    const prevStateId = lastStateIdRef.current;
    const searchParams = createSearchParams(location.search);
    const prevSearchParams = createSearchParams(prevLocation.current.search);

    lastStateIdRef.current = stateId;
    prevLocation.current = location;

    if (typeof stateId !== 'number') return;
    if (searchParams.has('u') || prevSearchParams.has('u')) return;

    if (prevStateId !== stateId) {
      // forward navigation, reset the scroll top
      if (stateId >= prevStateId) {
        (document.scrollingElement || document.body).scrollTo({ top: 0, behavior: 'instant' });
      }

      // focus (choose a best practice element (body, main, h1, interactive element??))
      document.querySelector('main')?.focus({ preventScroll: true });
    }
  }, [location]);
};
To also take into account:

It's important to remember to check how Search is affected by this: Do we support the history state of IDX (after each navigation, a state is pushed or popped). History is an array, so using .push() adds an entry. IDX represents the ID of the item.

Copy link

@langemike langemike left a comment

Choose a reason for hiding this comment

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

It is what it is, unfortunately. I think this is the best intermediate solution for now.

Copy link

@royschut royschut left a comment

Choose a reason for hiding this comment

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

It keeps it simple and straightforward. Let's do it!

@MelissaDTH MelissaDTH marked this pull request as draft April 17, 2024 15:46
@ChristiaanScheermeijer
Copy link
Member

@MelissaDTH let's submit this first and try the hook in a next PR.

@ChristiaanScheermeijer
Copy link
Member

@MelissaDTH are we ready for the PR? 😄

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.

5 participants