fix "Split bill- Own contact gets highlighted when you press down arrow and the highlight goes out of the screen when press down arrow" #10477

0xmiros · 2022-08-22T12:02:52Z

Details

original proposal:
#10127 (comment)
updated proposal:
#10127 (comment)
#10127 (comment)

Fixed Issues

$ #10127

Tests

Launch the app (desktop or web) and log in
Go to a Group conversation
Click actions > split bill > enter amount > split
Press down keys (arrow up or down) as many times as you can.
Verify that own contact is never highlighted and also that losing highlight never happens once highlighted.

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Launch the app (desktop or web) and log in
Go to a Group conversation
Click actions > split bill > enter amount > split
Press down keys (arrow up or down) as many times as you can.
Verify that own contact is never highlighted and also that losing highlight never happens once highlighted.

Verify that no errors appear in the JS console

Screenshots

Web

web.mov

Mobile Web

mweb.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mp4

…ow and the highlight goes out of the screen when press down arrow"

mananjadhav · 2022-08-23T18:04:16Z

src/components/OptionsSelector/BaseOptionsSelector.js

@@ -144,13 +144,19 @@ class BaseOptionsSelector extends Component {
     */
    flattenSections() {
        const allOptions = [];
+        this.disabledOptionsIndexes = [];


any specific reason we're defining it here, rather than the constructor?

The state value allOptions can change from componentDidUpdate

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 100 to 109 in dbfd0eb

componentDidUpdate(prevProps) {

if (_.isEqual(this.props.sections, prevProps.sections)) {

return;

}

const newOptions = this.flattenSections();

const newFocusedIndex = this.props.selectedOptions.length;

// eslint-disable-next-line react/no-did-update-set-state

this.setState({

allOptions: newOptions,

We need to sync this.disabledOptionsIndexes with this.state.allOptions always

Yeah we reinit there its fine. Not asking to remove from flattenSection, instead asking to define the member variable with default value in the constructor.

It can be defined in constructor but need to empty array at the entry of flattenSections in the case when called from componentDidUpdate because items already added in constructor

mananjadhav · 2022-08-23T18:08:45Z

src/components/ArrowKeyFocusManager.js

-            // Wrap around to the bottom of the list
-            if (newFocusedIndex < 0) {
-                newFocusedIndex = this.props.maxIndex;
+            while (this.props.disabledIndexes.includes(newFocusedIndex)) {


Weird edge case question: What happens when all the indexes are added to disabledIndexes ?

Good catch! Infinite loop that case! I fixed it here

mananjadhav · 2022-08-23T18:14:36Z

src/components/OptionsSelector/BaseOptionsSelector.js

                focusedIndex={this.state.focusedIndex}
-                maxIndex={this.props.canSelectMultipleOptions ? this.state.allOptions.length : this.state.allOptions.length - 1}
+                maxIndex={this.state.allOptions.length - 1}


I am still not confident of the change, so we'll have to counter with the test so that nothing breaks.

The purpose of original code was to have a state of no highlighted options when multiple options type, which is the 2nd issue of this GH (the highlight goes out of the screen)
So this change removes this state and doesn't break any others.

Only one possible regression will be:

App/src/components/ArrowKeyFocusManager.js

Lines 29 to 31 in dbfd0eb

if (this.props.maxIndex < 1) {

return;

}

(when there is only 1 option in list)
This case, that single option will not be highlighted by arrow keys in current codebase.
If it's fine, leave what it is now.
If it's issue, we can fix it by removing that code

yeah lets fix it @vladnobenladen

Completely removing the code won't help.

@mananjadhav any issue with it?

Because we could pass any integer and it can cause if the index is negative. But we can ignore this change as I am also fixing it here #7846 (comment)

ok, makes sense in case user can pass any bad values like -1, -100 to maxIndex props.
added and pushed @mananjadhav

mananjadhav

@vladnobenladen 1 change and 2 questions. Can you please take a look?

mananjadhav · 2022-08-31T03:30:06Z

@vladnobenladen I am also working on an issue that uses ArrowKeyFocusManager. It turns out that updating this component will require testing a good amount of regression suite. I recommend we do test the same regression suite here as well. #7846 (comment) is list of regression tests to be checked.

mananjadhav · 2022-09-03T18:14:37Z

@vladnobenladen Do you have any update for me in terms of testing the regression suite?

0xmiros · 2022-09-05T02:48:15Z

@vladnobenladen Do you have any update for me in terms of testing the regression suite?

yes, I tested all regression suites:
Search Page, New Chat Page, New Group Page, IOU (Split bill, 1:1 request, Confirmation List, Currency selection), Workspace Invite
All pages work flawlessly in all platforms (both selection types - single / multiple)

mananjadhav · 2022-09-05T06:29:07Z

Thanks for the update @vladnobenladen, I am going to test this today and update here.

Quick thing, can you take the latest main branch changes?

mananjadhav · 2022-09-06T06:43:26Z

Quick thing, can you take the latest main branch changes?

@vladnobenladen bump, waiting for this to test the changes.

…to issue-10127

mananjadhav · 2022-09-06T12:15:32Z

@vladnobenladen I might have to ask you to pull main changes once more. A recent change in ArrowKeyFocusManager was added and lets make sure we test against the latest code for the component. Thanks for the patience.

…to issue-10127

0xmiros · 2022-09-06T14:28:00Z

@vladnobenladen I might have to ask you to pull main changes once more. A recent change in ArrowKeyFocusManager was added and lets make sure we test against the latest code for the component. Thanks for the patience.

ok, I noticed that change was this.props.maxIndex < 0 (previously < 1). I did same change in my PR so no conflicts

mananjadhav

@MonilBhavsar Changes look fine and test well.

🎀 👀 🎀
C+ reviewed

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- Any functional components have the displayName property
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

mananjadhav · 2022-09-08T17:34:59Z

@MonilBhavsar Quick bump

melvin-bot · 2022-09-09T09:15:39Z

@MonilBhavsar looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

MonilBhavsar · 2022-09-09T09:16:05Z

Checklist are completed, but looks like integration is broken.
So merging it!

OSBotify · 2022-09-09T15:26:54Z

🚀 Deployed to staging by @MonilBhavsar in version: 1.1.99-1 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

vladnobenladen added 2 commits August 17, 2022 23:29

fix "Split bill- Own contact gets highlighted when you press down arr…

0a220e3

…ow and the highlight goes out of the screen when press down arrow"

fix "highlight goes out of screen when press down arrow"

704d654

0xmiros requested a review from a team as a code owner August 22, 2022 12:02

melvin-bot bot requested review from mananjadhav and MonilBhavsar and removed request for a team August 22, 2022 12:02

mananjadhav reviewed Aug 23, 2022

View reviewed changes

mananjadhav suggested changes Aug 23, 2022

View reviewed changes

vladnobenladen added 2 commits August 23, 2022 12:59

handle case of all options are disabled

342481d

handle case of single option

d44c976

add back "maxIndex" check

4eae63c

NicMendonca mentioned this pull request Aug 31, 2022

[HOLD for payment 2022-09-20] [$250] Web/Desktop - Split bill- Own contact gets highlighted when you press down arrow and the highlight goes out of the screen when press down arrow @Puneet-here #10127

Closed

Merge branch 'main' of https://github.com/vladnobenladen/Expensify in…

d1e764a

…to issue-10127

0xmiros mentioned this pull request Sep 6, 2022

[$250] Web/Desktop - Chat - Unable to go to search bar using arrow keys #10705

Closed

Merge branch 'main' of https://github.com/vladnobenladen/Expensify in…

d66417b

…to issue-10127

mananjadhav approved these changes Sep 6, 2022

View reviewed changes

MonilBhavsar approved these changes Sep 9, 2022

View reviewed changes

MonilBhavsar merged commit 6300fee into Expensify:main Sep 9, 2022

melvin-bot bot added the Emergency label Sep 9, 2022

MonilBhavsar removed the Emergency label Sep 9, 2022

OSBotify mentioned this pull request Sep 9, 2022

Deploy Checklist: New Expensify 2022-09-09 #10916

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix "Split bill- Own contact gets highlighted when you press down arrow and the highlight goes out of the screen when press down arrow" #10477

fix "Split bill- Own contact gets highlighted when you press down arrow and the highlight goes out of the screen when press down arrow" #10477

0xmiros commented Aug 22, 2022 •

edited by MonilBhavsar

Loading

mananjadhav Aug 23, 2022

0xmiros Aug 23, 2022 •

edited

Loading

mananjadhav Aug 23, 2022

0xmiros Aug 23, 2022

mananjadhav Aug 23, 2022

0xmiros Aug 23, 2022

mananjadhav Aug 23, 2022

0xmiros Aug 23, 2022

mananjadhav Aug 25, 2022

0xmiros Aug 25, 2022

mananjadhav Aug 31, 2022

0xmiros Aug 31, 2022

mananjadhav Aug 31, 2022

0xmiros Aug 31, 2022

mananjadhav left a comment

mananjadhav commented Aug 31, 2022

mananjadhav commented Sep 3, 2022

0xmiros commented Sep 5, 2022

mananjadhav commented Sep 5, 2022 •

edited

Loading

mananjadhav commented Sep 6, 2022 •

edited

Loading

mananjadhav commented Sep 6, 2022

0xmiros commented Sep 6, 2022

mananjadhav left a comment •

edited

Loading

mananjadhav commented Sep 8, 2022

melvin-bot bot commented Sep 9, 2022

MonilBhavsar commented Sep 9, 2022

OSBotify commented Sep 9, 2022

	componentDidUpdate(prevProps) {
	if (_.isEqual(this.props.sections, prevProps.sections)) {
	return;
	}

	const newOptions = this.flattenSections();
	const newFocusedIndex = this.props.selectedOptions.length;
	// eslint-disable-next-line react/no-did-update-set-state
	this.setState({
	allOptions: newOptions,

fix "Split bill- Own contact gets highlighted when you press down arrow and the highlight goes out of the screen when press down arrow" #10477

fix "Split bill- Own contact gets highlighted when you press down arrow and the highlight goes out of the screen when press down arrow" #10477

Conversation

0xmiros commented Aug 22, 2022 • edited by MonilBhavsar Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

0xmiros Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mananjadhav left a comment

Choose a reason for hiding this comment

mananjadhav commented Aug 31, 2022

mananjadhav commented Sep 3, 2022

0xmiros commented Sep 5, 2022

mananjadhav commented Sep 5, 2022 • edited Loading

mananjadhav commented Sep 6, 2022 • edited Loading

mananjadhav commented Sep 6, 2022

0xmiros commented Sep 6, 2022

mananjadhav left a comment • edited Loading

Choose a reason for hiding this comment

PR Reviewer Checklist

mananjadhav commented Sep 8, 2022

melvin-bot bot commented Sep 9, 2022

MonilBhavsar commented Sep 9, 2022

OSBotify commented Sep 9, 2022

0xmiros commented Aug 22, 2022 •

edited by MonilBhavsar

Loading

0xmiros Aug 23, 2022 •

edited

Loading

mananjadhav commented Sep 5, 2022 •

edited

Loading

mananjadhav commented Sep 6, 2022 •

edited

Loading

mananjadhav left a comment •

edited

Loading