Skip to content

RDOIAT-522 - add changeFocusedOption method#30

Merged
OS-ruineves merged 6 commits intomainfrom
rdoiat-522-changeFocusedOption-method
Nov 18, 2022
Merged

RDOIAT-522 - add changeFocusedOption method#30
OS-ruineves merged 6 commits intomainfrom
rdoiat-522-changeFocusedOption-method

Conversation

@OS-ruineves
Copy link

Added changeFocusedOption that allows the dropdown on SS to focus perfect matchs when searching

@OS-ruineves OS-ruineves requested review from os-lmo and osplm November 16, 2022 17:10
Copy link

@os-lmo os-lmo left a comment

Choose a reason for hiding this comment

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

See my comments

@OS-ruineves OS-ruineves requested a review from os-lmo November 17, 2022 10:40

this.setState({
focusedOption: optionToFocus,
})
Copy link

@os-lmo os-lmo Nov 17, 2022

Choose a reason for hiding this comment

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

In the other focusOption function, we are also:

  1. Changing the the state of focusedValue to null
  2. Setting this.scrollToFocusedOptionOnUpdate = true;
    https://github.com/OutSystems/react-select/blob/main/src/Select.js#L594

Aren't they needed here also?

Copy link
Author

Choose a reason for hiding this comment

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

  1. focusedValue is used on multi select dropdowns, I had removed it while debugging, value operations change focusedOption to null, and option operations change focusedValue to null, so I will be adding it back again
  2. yes, this should improve the experience on dropdowns with a bigger number of options

Copy link

Choose a reason for hiding this comment

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

Also, what does the announceAriaLiveContext that is being called in other operations? Seems to update some kind of instructions. What are we losing not calling that here?

Copy link
Author

Choose a reason for hiding this comment

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

announceAriaLiveContext and announceAriaLiveSelection are disabled in the web-components code, with the allowScreenReaders prop

@os-lmo
Copy link

os-lmo commented Nov 17, 2022

Can you check if our pipeline is executing the automatic tests? In that case, can you create a test here - https://github.com/OutSystems/react-select/blob/main/src/__tests__/Select.test.js

@OS-ruineves OS-ruineves requested a review from os-lmo November 17, 2022 12:50
src/Select.js Outdated
return;
}

let optionToFocus = options.find(e => e.label == opt);
Copy link

Choose a reason for hiding this comment

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

this can be a const as well

@OS-ruineves OS-ruineves merged commit 70cd82b into main Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants