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

[select] feat(QueryList): add ref to ItemRendererProps #5815

Merged
merged 5 commits into from
Feb 13, 2023

Conversation

shim-flounce
Copy link
Contributor

@shim-flounce shim-flounce commented Dec 30, 2022

Fixes #3369

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

See #3369 (comment)

I didn't add any tests because it seems like there's no tests for scrolling, and I wasn't sure how to best write such a test:

describe("scrolling", () => {
it("brings active item into view");
});

Reviewers should focus on:

getActiveElement being exposed as desired

Screenshot

@shim-flounce
Copy link
Contributor Author

@adidahiya Apologies for the direct ping, but do you have any thoughts on the proposed change?

@adidahiya
Copy link
Contributor

The code seems reasonable so far, can you demonstrate usage of this new prop in the docs somehow? I think you'd need to design a new example option which uses itemListRenderer in a meaningful way. I'd prefer to add new props / functionality only if it's been unit tested or demonstrated clearly in docs-app.

@shim-flounce
Copy link
Contributor Author

The code seems reasonable so far, can you demonstrate usage of this new prop in the docs somehow? I think you'd need to design a new example option which uses itemListRenderer in a meaningful way. I'd prefer to add new props / functionality only if it's been unit tested or demonstrated clearly in docs-app.

Makes sense! To be honest I just blindly followed the suggestion in the linked issue, but I think it’s a bit awkward to use (although it does satisfy my use case). I’ll try to see if there’s a better way to solve this and update the PR (might be a couple of weeks as I’m going on vacation though). Thanks for your review anyway!

@shim-flounce
Copy link
Contributor Author

shim-flounce commented Feb 6, 2023

@adidahiya I've updated the Select example to include a grouped option that uses this new prop. That being said, I think it would be better to enhance ItemRendererProps with a ref and let users wire it up to the right element?

@shim-flounce shim-flounce changed the title feat(QueryList): add getActiveElement prop feat(QueryList): add ref to ItemRendererProps prop Feb 8, 2023
@shim-flounce shim-flounce changed the title feat(QueryList): add ref to ItemRendererProps prop feat(QueryList): add ref to ItemRendererProps Feb 8, 2023
@@ -419,10 +428,12 @@ export class QueryList<T> extends AbstractComponent2<QueryListProps<T>, IQueryLi
if (this.itemsParentRef != null) {
if (isCreateNewItem(activeItem)) {
const index = this.isCreateItemFirst() ? 0 : this.state.filteredItems.length;
return this.itemsParentRef.children.item(index) as HTMLElement;
return this.itemRefs.get(index) ?? (this.itemsParentRef.children.item(index) as HTMLElement);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fallback is for backwards compat (if a user didn't wire up the new ref to elementRef on the MenuItem for example)

@shim-flounce
Copy link
Contributor Author

@adidahiya I've changed the approach but kept the grouped example for you to verify scroll behaviour. I'm happy to drop the example though to keep the diff small.

@@ -394,6 +396,13 @@ export class QueryList<T> extends AbstractComponent2<QueryListProps<T>, IQueryLi
index,
modifiers,
query,
ref: node => {
Copy link
Contributor Author

@shim-flounce shim-flounce Feb 8, 2023

Choose a reason for hiding this comment

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

@@ -422,7 +431,9 @@ export class QueryList<T> extends AbstractComponent2<QueryListProps<T>, IQueryLi
return this.itemsParentRef.children.item(index) as HTMLElement;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strictly speaking, we could also wire up a ref to createNewItemRenderer so that we can easily look it up, but I think even without doing so 0 and this.state.filteredItems.length should be good enough as indices.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

nicely done @shim-flounce, I can clearly see this fixes the linked issue in the docs preview build:

2023-02-13 14 18 46

2023-02-13 14 19 04

@adidahiya adidahiya changed the title feat(QueryList): add ref to ItemRendererProps [select] feat(QueryList): add ref to ItemRendererProps Feb 13, 2023
@adidahiya adidahiya merged commit 96c9143 into palantir:develop Feb 13, 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.

[Select] custom itemListRenderer breaks scrollToActiveItem
2 participants