fix(combobx): display selected item when initally opened #11427
Merged
+57
−14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
bug
josercarcamo
added
the
pr ready for visual snapshots
driskull
approved these changes
Jan 31, 2025
| return; | ||
| } | ||
|
|
||
| const height = this.calculateScrollerHeight(activeItem); | ||
| const height = this.calculateScrollerHeight(item); |
There was a problem hiding this comment.
@josercarcamo instead of calculating the height, can you try the following:
item.scrollIntoView({ block: "nearest" });I'm not sure calculating the offset is necessary.
josercarcamo
added
pr ready for visual snapshots
github-actions
bot
added this to the 2025-02-06 - 3.0.0 Feb Breaking Change Release milestone
jcfranco
reviewed
Feb 1, 2025
| private scrollToActiveItem(): void { | ||
| const activeItem = this.filteredItems[this.activeItemIndex]; | ||
| private scrollToActiveOrSelectedItem(scrollToSelected = false): void { | ||
| const item = |
There was a problem hiding this comment.
FYI, you can use the optional chaining operator to simplify these types of checks:
scrollToSelected && this.selectedItems?.lengthWe might be able to automate this with https://typescript-eslint.io/rules/prefer-optional-chain/.
jcfranco
reviewed
Feb 1, 2025
There was a problem hiding this comment.
Awesome, stuff! @josercarcamo! I've added some follow-up comments.
Could you also fix the typo in the PR title and follow these steps to amend the conventional commit entry (see example)?
| it("shows the selected item when initially opened", async () => { | ||
| const page = await newE2EPage(); | ||
|
|
||
| await page.setContent(` |
There was a problem hiding this comment.
Can you follow this up by:
- using the
htmlformatting helper (example) - Removing any attributes not needed for the test –
iddoesn't seem to be used and optionallyplaceholdersince the test name and assertion convey this - adding a test for the multiple selection case (one should suffice) for extra coverage
- renaming
item1to something that matches the test
?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related Issue: #9890
Summary
Scroll to display selected item when combobox is initially opened.