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

DON-1092 – refine post-search scroll behaviour #1773

Merged
merged 5 commits into from
Nov 26, 2024
Merged

DON-1092 – refine post-search scroll behaviour #1773

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f47a4cb
DON-1092 – give post-search scroll position change enough time to go …
NoelLH Nov 26, 2024
bb8e736
DON-1092 – fix no-op navigation with scroll position impact
NoelLH Nov 26, 2024
a175d82
Minor performance & code style tweaks
NoelLH Nov 26, 2024
4a1d962
DON-1092 – fix Explore typo
NoelLH Nov 26, 2024
1bec863
DON-1092 – update comments about our delayed scrolls
NoelLH Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/app/campaign-details/campaign-details.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ export class CampaignDetailsComponent implements OnInit, OnDestroy {

private timer: any; // State update setTimeout reference, for client side when donations open soon

ngOnInit() {
this.campaign = this.route.snapshot.data.campaign;
this.setSecondaryProps(this.campaign);
}

constructor(
private datePipe: DatePipe,
private location: Location,
Expand All @@ -52,11 +57,6 @@ export class CampaignDetailsComponent implements OnInit, OnDestroy {
});
}

ngOnInit() {
this.campaign = this.route.snapshot.data.campaign;
this.setSecondaryProps(this.campaign);
}

ngOnDestroy() {
if (isPlatformBrowser(this.platformId) && this.timer) {
window.clearTimeout(this.timer);
Expand Down
14 changes: 10 additions & 4 deletions src/app/explore/explore.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export class ExploreComponent implements OnDestroy, OnInit {

// Angular scrolls automatically, using setTimeout to delay this scroll to a later task so this gets to
// set the position the page is left in.
setTimeout(() => positionMarker?.scrollIntoView({}), 0);
setTimeout(() => positionMarker?.scrollIntoView({ behavior: 'smooth', block: 'start' }), 200);
}
});
}
Expand Down Expand Up @@ -192,8 +192,14 @@ export class ExploreComponent implements OnDestroy, OnInit {
* Update the browser's query params when a sort or filter is applied.
*/
private setQueryParams() {
this.router.navigate(['explore'], {
queryParams: this.searchService.getQueryParams(this.defaultSort),
});
const nextQueryParams = this.searchService.getQueryParams(this.defaultSort);
if (JSON.stringify(this.route.snapshot.queryParams) === JSON.stringify(nextQueryParams)) {
// Don't navigate at all if no change in query params. This saves us from inconsistencies
// later such as scroll adjustment kicking in only when the router params actually changed,
// and saves giving the browser needless work to do.
return;
}

this.router.navigate(['explore'], { queryParams: nextQueryParams });
}
}
2 changes: 1 addition & 1 deletion src/app/meta-campaign/meta-campaign.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
[mainMessage]="tickerMainMessage"
>
<div slot="ticker-items">
@for (tickerItem of tickerItems; track tickerItem) {
@for (tickerItem of tickerItems; track tickerItem.label) {
<biggive-totalizer-ticker-item
[figure]="tickerItem.figure"
[label]="tickerItem.label">
Expand Down
14 changes: 10 additions & 4 deletions src/app/meta-campaign/meta-campaign.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ export class MetaCampaignComponent implements AfterViewChecked, OnDestroy, OnIni

// Angular scrolls automatically, using setTimeout to delay this scroll to a later task so this gets to
// set the position the page is left in.
Copy link
Contributor

Choose a reason for hiding this comment

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

probably worth changing this comment, "later task" made sense when delay was set at zero, now we're delaying it more than that. Not sure exactly what it is that we're waiting the 200ms for. Maybe something like this?

Suggested change
// Angular scrolls automatically, using setTimeout to delay this scroll to a later task so this gets to
// set the position the page is left in.
// Angular scrolls automatically, using setTimeout to delay this scroll to happen after the automatic scroll

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'm also not sure exactly, but think it's connected to the router action and possibly to its scrollPositionRestoration feature which we have set to true.

Updated the comments to match my understanding better at 1bec863

setTimeout(() => positionMarker?.scrollIntoView({}), 0);
setTimeout(() => positionMarker?.scrollIntoView({ behavior: 'smooth', block: 'start' }), 200);
}
});
}
Expand Down Expand Up @@ -381,9 +381,15 @@ export class MetaCampaignComponent implements AfterViewChecked, OnDestroy, OnIni
* Update the browser's query params when a sort or filter is applied.
*/
private setQueryParams() {
this.router.navigate([], {
queryParams: this.searchService.getQueryParams(this.getDefaultSort()),
});
const nextQueryParams = this.searchService.getQueryParams(this.getDefaultSort());
if (JSON.stringify(this.route.snapshot.queryParams) === JSON.stringify(nextQueryParams)) {
// Don't navigate at all if no change in query params. This saves us from inconsistencies
// later such as scroll adjustment kicking in only when the router params actually changed,
// and saves giving the browser needless work to do.
return;
}

this.router.navigate([], { queryParams: nextQueryParams });
}

private listenForRouteChanges() {
Expand Down
Loading