-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix: reset scroll on ExtraParamsChanged by default #1423
base: main
Are you sure you want to change the base?
fix: reset scroll on ExtraParamsChanged by default #1423
Conversation
6100660
to
73da49a
Compare
Thanks for submitting the PR @alonsogarciapablo! We will review it as soon as we can. |
We're finally solving the issue in |
Hi @alonsogarciapablo, |
Hey @annacv, we're using the Could we also trigger an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alonsogarciapablo,
The ExtraParams
component is not intended for changing extra-params dynamically, it is just a “register” of all the params that will be used when the application is initialized. IMO, the UserChangedExtraParams
event should not be attached by default to the component in the library, but for your use case, reacting to the ExtraParams
's computed prop values's change by emitting the UserChangedExtraParams
event would be a good approach.
Anyway, we can argue about changing the ScrollMixin
behavior and the events it is listening to. In this case, we can remove the UserChangedExtraParams
event from the list and only listen to ExtraParamsChanged
. At the end, whenever theUserChangedExtraParams
event will be emitted, the ExtraParamsXStoreModule
will emit ExtraParamsChanged
reacting to the state change.
https://jira.kroger.com/jira/browse/DCPPERS-12567
Motivation and context
In the Playboard, we use the
ExtraParams
component (wrapped here). We often modify theseextraParams
programmatically.We recently discovered a "bug" that the scroll of the
ResultList
(which we also use) wasn't getting reseted, causing and extra request to the search endpoint (for the second page or results) and the incorrect results displayed in the list 😢The
ScrollMixin
component was already "listening" to theUserChangedExtraParams
event (triggered byRenderlessExtraParam
) and resetting the scroll of the scrollable element, but it wasn't "listening" to theExtraParamsChanged
event (triggered byExtraParams
).This PR basically adds
ExtraParamsChanged
to the "default" list of events that should cause the scroll to be reseted.Type of change
What is the destination branch of this PR?
Main
How has this been tested?
Tests performed according to testing guidelines:
I tested this manually by adding the following to
packages/x-components/src/views/home/Home.vue
:Without this fix (aka HOW TO REPRODUCE the issue):
With this fix:
Checklist: