Skip to content

[Focus] and [Filters] Fixing initial reference focusing #2871

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 22, 2020
Merged

[Focus] and [Filters] Fixing initial reference focusing #2871

merged 1 commit into from
Apr 22, 2020

Conversation

@posth
Copy link
Contributor

@posth posth commented Mar 24, 2020 •
edited
Loading

WHY are these changes introduced?

Fixes #2741

There are two issues with the current behavior:

  • When toggling open a new filter, it does not focus the first selectable input (it does so after selecting and re-toggling)
  • When selecting an input on a newly toggled filter, it will force a focus away

WHAT is this pull request doing?

The reference being passed to the <Focus /> component from <Filters /> was the direct reference to it's current value, which would be a stale reference by the time the component is mounted and rendered, forcing that focused element to be "one step behind."

This PR adds an union type to be passed to the root of <Focus />, which is the entire React.RefObject, and lets the Focus component grab it's current node instead, so that we are sure that the current node we want to focus on is the node value of the current reference we passed.

Choosing to union the type onto the prop for backwards compatibility and not break current consumers.

How to 🎩

To 🎩

  • Run yarn dev
  • Select the Filters component and then Filter with resource list
  • This renders a default resource list in the sandbox
  • Toggle the More filters
  • Toggle open the Account status
  • It will focus on the first selectable input (did not do that before)
  • Select another option and it will stay focused on that input (before it would focus the first input)
filtersFocusFix

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@posth posth force-pushed the filters_focus_fix branch from 0de3b51 to b86245d Compare March 24, 2020 16:10
@github-actions
Copy link
Contributor

github-actions bot commented Mar 24, 2020 •
edited
Loading

🟡 This pull request modifies 4 files and might impact 9 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:
All files potentially affected (total: 9)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Filters/Filters.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Focus/Focus.tsx (total: 9)

Files potentially affected (total: 9)

🧩 src/components/Focus/tests/Focus.test.tsx (total: 0)

Files potentially affected (total: 0)

@posth posth force-pushed the filters_focus_fix branch 5 times, most recently from 97fae92 to 0280523 Compare March 24, 2020 17:49
@posth posth requested review from chloerice and loic-d March 24, 2020 18:20
posth
posth commented Mar 24, 2020
View reviewed changes
src/components/Focus/Focus.tsx
children?: React.ReactNode;
disabled?: boolean;
root: HTMLElement | null;
root: React.RefObject<HTMLElement> | HTMLElement | null;
Copy link
Contributor Author

@posth posth Mar 24, 2020

Choose a reason for hiding this comment

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

I feel like passing the entire ref is safer, but this would break the API of this component - would it make sense to go towards passing the entire ref and add a warning of deprecation for passing only a current element?

Copy link
Member

@kyledurand kyledurand Apr 16, 2020

Choose a reason for hiding this comment

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

I'm a fan of this and would prefer to remove HTMLElement altogether but that can be done in another PR since it would be a breaking change.

Copy link
Contributor Author

@posth posth Apr 17, 2020

Choose a reason for hiding this comment

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

Because it's a breaking change, is there a way in Polaris to have a warning for future deprecations?

@posth posth force-pushed the filters_focus_fix branch 2 times, most recently from acd2202 to a9f551a Compare March 31, 2020 13:04
loic-d
loic-d reviewed Apr 1, 2020
View reviewed changes
loic-d
loic-d reviewed Apr 1, 2020
View reviewed changes
loic-d
loic-d approved these changes Apr 1, 2020
View reviewed changes
Copy link
Contributor

@loic-d loic-d left a comment

Choose a reason for hiding this comment

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

🎩 looks great. Would be great to get a second 👀 from a Polaris expert cc/ @chloerice 🙌

@posth posth force-pushed the filters_focus_fix branch 3 times, most recently from 279a875 to 596bc65 Compare April 6, 2020 13:01
@chloerice
Copy link
Member

chloerice commented Apr 6, 2020 •
edited
Loading

Hey @posth sorry I missed this, will tophat this and #2880 this afternoon!

@posth posth force-pushed the filters_focus_fix branch from 596bc65 to 22c497b Compare April 14, 2020 17:37
kyledurand
kyledurand requested changes Apr 16, 2020
View reviewed changes
Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Looks good I'd just prefer type guarding vs if statement with casting

src/components/Focus/Focus.tsx
children?: React.ReactNode;
disabled?: boolean;
root: HTMLElement | null;
root: React.RefObject<HTMLElement> | HTMLElement | null;
Copy link
Member

@kyledurand kyledurand Apr 16, 2020

Choose a reason for hiding this comment

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

I'm a fan of this and would prefer to remove HTMLElement altogether but that can be done in another PR since it would be a breaking change.

@chloerice chloerice removed their request for review April 16, 2020 15:48
@posth posth force-pushed the filters_focus_fix branch 2 times, most recently from 3f5aaaa to c7df53c Compare April 17, 2020 18:41
@posth posth force-pushed the filters_focus_fix branch from c7df53c to 22c0331 Compare April 22, 2020 12:46
kyledurand
kyledurand approved these changes Apr 22, 2020
View reviewed changes
Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Nice 👍

src/components/Focus/Focus.tsx
Comment on lines +32 to +36
function isRef(
ref: React.RefObject<HTMLElement> | HTMLElement,
): ref is React.RefObject<HTMLElement> {
return (ref as React.RefObject<HTMLElement>).current !== undefined;
}
Copy link
Member

@kyledurand kyledurand Apr 22, 2020

Choose a reason for hiding this comment

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

👏

@posth posth merged commit 8c3baa3 into master Apr 22, 2020
@posth posth deleted the filters_focus_fix branch April 22, 2020 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@loic-d loic-d loic-d approved these changes

@kyledurand kyledurand kyledurand approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Filters / ChoiceList filter does not focus the right option

4 participants

@posth @chloerice @loic-d @kyledurand