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

Create PriceRangeFilter component #208

Merged
merged 1 commit into from
Nov 21, 2022
Merged

Conversation

lordofthecactus
Copy link
Contributor

@lordofthecactus lordofthecactus commented Nov 18, 2022

Part of #154

This creates a PriceRangeFilter

image

Feedback I need

  • Seems very react rather than remix the PriceRangeFilter because of the debouncing, wdyt of this?

@lordofthecactus lordofthecactus self-assigned this Nov 18, 2022
@lordofthecactus lordofthecactus requested a review from a team November 18, 2022 15:57
Copy link
Contributor

@rennyG rennyG left a comment

Choose a reason for hiding this comment

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

Copy looks good to me! Can't scrutinize code obvs

const location = useLocation();

const filterMarkup = (filter: Filter, option: Filter['values'][0]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe wrap on useCallback, [] to save some memory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean save some re-rendering time? I looked into it a bit after your suggestions. Seems params will change always and re-render will happen anyway. Fixing this will end up making the code complicated. I'd leave this is as it is for now and revisit if necessary.

return <PriceRangeFilter min={min} max={max} />;

default:
const to = getFilterLink(
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to double check it supports /$lang/ urls too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works!

if (maxPrice !== '') price.max = maxPrice;

const newParams = filterInputToParams('PRICE_RANGE', {price}, params);
window.location.href = `${location.pathname}?${newParams.toString()}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

window.location.href will cause a full render of root loaders and what not. I'd use fetcher.load('updatedUrl') instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll tackle this in next PR, we are looking to avoid full page reloads

Copy link
Contributor

@juanpprieto juanpprieto left a comment

Choose a reason for hiding this comment

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

👍🏼 nice one!! left some comments. Might be interesting to check with Jacob or Matt to see if there's a way to avoid debouncing.

@lordofthecactus
Copy link
Contributor Author

@juanpprieto

👍🏼 nice one!! left some comments. Might be interesting to check with Jacob or Matt to see if there's a way to avoid debouncing.
Agree, I'm waiting to review of the design by Adam

@lordofthecactus
Copy link
Contributor Author

@juanpprieto

👍🏼 nice one!! left some comments. Might be interesting to check with Jacob or Matt to see if there's a way to avoid debouncing.

Agree, I'm waiting to review of the design by Adam

@lordofthecactus lordofthecactus merged commit 294c6c8 into main Nov 21, 2022
@lordofthecactus lordofthecactus deleted the price-range-filter branch November 21, 2022 12:41
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.

3 participants