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

LB-619: Improve page navigation on History page #900

Merged
merged 11 commits into from
Jun 9, 2020

Conversation

ishaanshah
Copy link
Collaborator

@ishaanshah ishaanshah commented Jun 2, 2020

Problem

  • If an user directly goes to a page /user/u/history?page=4&range=all_time&entity=artist, they see the page flash with the contents of page 1, before being replaced with the contents of page 4. Ideally the page should just show the data of page 4 immediately.
  • If an user goes to a page, and then click the next button, the URL updates, and the data updates, but if they click the browser's back button, the URL changes but the data on the page doesn't change.

Jira Ticket - LB-619

Solution

Using the popstate event to restore the data when back or forward button is pressed solves the issue.

@ishaanshah ishaanshah marked this pull request as draft June 3, 2020 02:58
@ishaanshah ishaanshah changed the title [WIP] Improve page navigation on History page LB-619: Improve page navigation on History page Jun 3, 2020
@ishaanshah ishaanshah marked this pull request as ready for review June 4, 2020 07:19
Copy link
Contributor

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Well done on getting the navigation to work.
The component seems a bit cleaner too.

I can't test this locally (no stats), but I'm happy to do a round of testing on beta.

listenbrainz/webserver/static/js/src/stats/UserHistory.tsx Outdated Show resolved Hide resolved
@ishaanshah
Copy link
Collaborator Author

Well done on getting the navigation to work.
The component seems a bit cleaner too.

I can't test this locally (no stats), but I'm happy to do a round of testing on beta.

You can change the API URL in your config file to api.listenbrainz.org so that it would work without stats being present.

@paramsingh
Copy link
Collaborator

You can change the API URL in your config file to api.listenbrainz.org so that it would work without stats being present.

We should document this somewhere.

Copy link
Collaborator

@paramsingh paramsingh left a comment

Choose a reason for hiding this comment

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

Deployed on beta.

  • The first load starts with Top S of the Year 2020 right now, we'll probably fix it with the dual loading thing MrMonkey and alastair are working on.
  • The Graph transitions seem to have gotten weird, there's some invalid state between them. Maybe when we change state, we should set a loading: true and in the render function when loading is true show a Loading spinner.

@ishaanshah
Copy link
Collaborator Author

@shivam-kapila
Copy link
Collaborator

@ishaanshah Can you please enable link sharing access for the loader demo.

Copy link
Collaborator

@paramsingh paramsingh 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 to me overall. Just a few comments.

docs/dev/devel-env.rst Outdated Show resolved Hide resolved
docs/dev/devel-env.rst Outdated Show resolved Hide resolved
@paramsingh paramsingh merged commit ab71df5 into metabrainz:master Jun 9, 2020
@ishaanshah ishaanshah deleted the back_button branch June 10, 2020 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants