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.

Sign up for GitHub

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

Jump to bottom

feat(filter-menu): add header with search #2330

Merged
merged 7 commits into from
Nov 22, 2024
Merged

feat(filter-menu): add header with search #2330

merged 7 commits into from
Nov 22, 2024

Conversation

LuLaValva
Copy link
Member

Description

  • Add search header

Context

  • @ianmcburnie and I decided offline that filtering should happen outside of eBayUI, because:
    1. There are likely multiple types of custom filtering (i. e. case sensitive, beginning of term vs within term)
    2. Simple cases that would likely be a good candidate for eBayUI to do the filtering should likely not even have a search filter to begin with
    3. Network requests are impossible to handle in eBayUI

Screenshots

image image

@LuLaValva LuLaValva self-assigned this Nov 19, 2024
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 19, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: e92122a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@ebay/ebayui-core Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

agliga
agliga reviewed Nov 20, 2024
View reviewed changes
src/components/ebay-filter-menu/component.ts
Comment on lines +67 to +68
this.state.searchTerm = (e.target as HTMLInputElement).value;
this.emit("search-change", this.state.searchTerm);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add an option to filter the results? Like we have with combobox

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 also iterate on this if needed

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's overengineering to add our own filtering option, see the issue description for the results of a discussion with @ianmcburnie

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is urgent, and is a "nice to have." @LuLaValva , can you create a separate issue for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Added #2331

ArtBlue
ArtBlue requested changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Looks good overall. One minor bug and a UX gap that we may not be able to do anything at the moment. I just wanted to document it.

src/components/ebay-filter-menu/examples/with-search.marko Show resolved Hide resolved
agliga
agliga reviewed Nov 21, 2024
View reviewed changes
src/components/ebay-filter-menu/examples/with-search.marko Outdated
};
}

handleSearchChange(value: string) {
this.state.searchTerm = value.toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not want to save this as a state variable and pass it into the component?
I think we might in case it gets fully rerendered and state is lost?

Otherwise it looks good to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is already protected against rerenders in the <ebay-filter-menu> component because it stores searchTerm in its state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could there be a usecase where the developer of this component wants to pass in and sync up the state of the search
I'm thinking of page navigations or such.
Generally I think its a good idea to emit the value of the search and then update a parents state and pass that in as input.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't think of a usecase, but I guess there's no harm in adding it. I just pushed a commit that includes this functionality.

ArtBlue
ArtBlue approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Looks great! Let's just create an issue for that feature we can iterate on in the future.

@LuLaValva LuLaValva mentioned this pull request Nov 22, 2024
Open
@LuLaValva LuLaValva merged commit 9ebb7c9 into 14.5.0 Nov 22, 2024
5 checks passed
@LuLaValva LuLaValva deleted the filter-menu-search-bar branch November 22, 2024 18:28
@github-actions github-actions bot mentioned this pull request Nov 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agliga agliga agliga left review comments

@ArtBlue ArtBlue ArtBlue approved these changes

Assignees

@LuLaValva LuLaValva

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LuLaValva @ArtBlue @agliga