Skip to content
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.

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

[#2864] va-select hovered position fix #3129

Merged

Conversation

aluarius
Copy link
Contributor

@aluarius aluarius commented Mar 7, 2023

Closes: #2964

Description

  • multiple updates of option selection logic;
  • hovered option for selectedTopShown state after selection loses her hovered state, it goes to the previous option;

Need QA, expecially of keyboard navigation.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

@aluarius aluarius self-assigned this Mar 7, 2023
Copy link
Collaborator

@rustem-nasyrov rustem-nasyrov left a comment

Choose a reason for hiding this comment

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

When I selected and deselect options by enter/return key multiple times I get this result:
image

return findNextActiveOption(currentOptionIndex.value - 1, true)
}

return undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

Redundant undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not redundant -> eslint rules forces computed to have obvious return value.

Issue with 0 not related to this pr, will move it separately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about null. It is not undefined, it is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not empty, it doesn't exist, undefined. Moreover, findNextActiveOption may return undefined if find method will find nothing. I don't think it's a good case to have one computed to return both null and undefined in same situations.

@aluarius aluarius requested a review from rustem-nasyrov March 7, 2023 12:46
# Conflicts:
#	packages/ui/src/components/va-select/VaSelect.vue
@m0ksem m0ksem merged commit 4a73ccb into epicmaxco:develop Mar 9, 2023
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.

[VaSelect]: selectedTopShown mode should keep hovered option under cursor after selection
3 participants