Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-list]Fix for terra-menu console error #3935

Merged
merged 3 commits into from
Oct 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/terra-list/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Fixed
* Fixed the console error appearing in the terra-menu examples while keyboard navigation.
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the errors?

Copy link
Contributor Author

@ashishkumbhare116 ashishkumbhare116 Oct 12, 2023

Choose a reason for hiding this comment

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

Following is the error getting while navigating through arrow key (https://engineering.cerner.com/terra-framework/components/cerner-terra-framework-docs/menu/menu) :-
Uncaught TypeError: Cannot read properties of undefined (reading 'focus')

Copy link
Contributor

Choose a reason for hiding this comment

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

We can rephrase this to:

Fixed _property is undefined_ error while navigating with a keyboard.


## 4.64.0 - (October 3, 2023)

* Added
Expand Down
8 changes: 6 additions & 2 deletions packages/terra-list/src/List.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,17 @@ const List = ({
case KeyCode.KEY_UP: {
event.preventDefault();
const previousIndex = currentIndex > 0 ? currentIndex - 1 : lastIndex;
listItems[previousIndex].focus();
if (listItems[previousIndex]) {
listItems[previousIndex].focus();
}
Comment on lines +165 to +167
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes should be tested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes have tested by creating the tar file with this change on terra-menu . For reference please check the screen recording attached in PR description for pre and post implementation .

Copy link
Contributor

@sdadn sdadn Oct 13, 2023

Choose a reason for hiding this comment

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

Sorry, bad wording on my part. I trust that this was tested before submitting  🙂

What I meant was that we should write tests for these to test the condition and any potential edge cases. It can be a unit test or a wdio test.

Copy link
Contributor Author

@ashishkumbhare116 ashishkumbhare116 Oct 16, 2023

Choose a reason for hiding this comment

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

@sdadn For this particular scenario not being able to create test :- These functionality can be hard because not being able to assert the focus invocation on null object ,which is making hard to write the test case to justify the changes made on List.jsx.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I thought you could test the condition but that makes sense. If it's not possible, then the screen recording should be good enough. Thanks for trying!
+1

break;
}
case KeyCode.KEY_DOWN: {
event.preventDefault();
const nextIndex = currentIndex < lastIndex ? currentIndex + 1 : 0;
listItems[nextIndex].focus();
if (listItems[nextIndex]) {
listItems[nextIndex].focus();
}
break;
}
default:
Expand Down
Loading