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

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

merged 3 commits into from
Oct 18, 2023

Conversation

ashishkumbhare116
Copy link
Contributor

@ashishkumbhare116 ashishkumbhare116 commented Oct 12, 2023

Summary

What was changed:
Implemented the condition before focus to handle undefined value condition

Why it was changed:
User is getting Errors when navigating using arrow keys on terra-menu.

Testing

This change was tested using:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review

Additional Details

This PR resolves:

UXPLATFORM-9738


Pre implementation:-

Screen.Recording.2023-10-12.at.12.30.57.PM.mov

Post implementation:-

Screen.Recording.2023-10-12.at.12.33.04.PM.mov

Thank you for contributing to Terra.
@cerner/terra

@ashishkumbhare116 ashishkumbhare116 changed the title terra menu console error fix [terra-list]Fix for terra-menu console error Oct 12, 2023
@ashishkumbhare116 ashishkumbhare116 marked this pull request as ready for review October 12, 2023 08:23
@ashishkumbhare116 ashishkumbhare116 requested a review from a team as a code owner October 12, 2023 08:23
Comment on lines +165 to +167
if (listItems[previousIndex]) {
listItems[previousIndex].focus();
}
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

@@ -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.

@github-actions github-actions bot temporarily deployed to preview-pr-3935 October 17, 2023 14:13 Destroyed
@supreethmr supreethmr merged commit 2b47580 into main Oct 18, 2023
21 checks passed
@supreethmr supreethmr deleted the menu-logerror branch October 18, 2023 05:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants