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
Migrate Entity Pages to SPA #2769
Conversation
Hello @anshg1214! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-03-25 18:07:46 UTC |
…erver into spa-entity-pages
…inz/listenbrainz-server into spa-entity-pages
…erver into spa-entity-pages
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.
I did a thorough look through the PR, trying to get the hang of using ReactRouter as I go along...
Seems to be working great so far! I haven't tested everything since I want to do that on test.LB with everything else set up, but what I've tested works really well.
Finally being able to navigate throughout the website without reloading the whole page feels absolutely great !
@@ -21,26 +21,26 @@ | |||
</a> |
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.
Is this template file still in use?
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.
Yes, this template is still being used by auth.html
for the AudioScrobbler API
listenbrainz-server/listenbrainz/webserver/views/api_compat.py
Lines 44 to 78 in cd0fcf9
def api_auth_approve(): | |
""" Authenticate the user token provided. | |
""" | |
user = User.load_by_name(db_conn, current_user.musicbrainz_id) | |
if "token" not in request.form: | |
return render_template( | |
"user/auth.html", | |
user_id=current_user.musicbrainz_id, | |
msg="Missing required parameters. Please provide correct parameters and try again." | |
) | |
token = Token.load(db_conn, request.form['token']) | |
if not token: | |
return render_template( | |
"user/auth.html", | |
user_id=current_user.musicbrainz_id, | |
msg="Either this token is already used or invalid. Please try again." | |
) | |
if token.user: | |
return render_template( | |
"user/auth.html", | |
user_id=current_user.musicbrainz_id, | |
msg="This token is already approved. Please check the token and try again." | |
) | |
if token.has_expired(): | |
return render_template( | |
"user/auth.html", | |
user_id=current_user.musicbrainz_id, | |
msg="This token has expired. Please create a new token and try again." | |
) | |
token.approve(db_conn, user.name) | |
return render_template( | |
"user/auth.html", | |
user_id=current_user.musicbrainz_id, | |
msg="Token %s approved for user %s, press continue in client." % (token.token, current_user.musicbrainz_id) | |
) |
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.
Update: The Auth page has also been migrated to SPA. Now Navbar.html is being used in error/{code}.html
files.
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.
OK, let's keep those for a follow-up PR, they are not as important
…erver into spa-entity-pages
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.
Looking better and better!
Only some small details remaining now!
Found a couple of inconsistencies while thoroughly testing on test.LB:
- When I first load test.LB and get redirected to my dashboard, the Dashboard link in the navbar is not active
- Similarly, clicking on the secondary navbar items in my dashboard makes the dashboard link not underlined. Only the "listens" tab underlines it
- On the artist page, clicking on one of the similar artist names at the bottom doesn't update some of the information; the artist name in the page, page title, tracklist and probably others don't update, but I think this is due to some props update logic missing in the artist page (which I didn't implement at the time since we didn't have that seamless navigation)
- The listening-now page should not show the navbar or footer. Compare https://listenbrainz.org/listening-now/ and https://test.listenbrainz.org/listening-now/ while playing some music
- I sometimes get an API rate limit error (see below in this comment about that), the issue being that the error is not caught and I don't think the error boundary is doing the right thing: The whole page is replaced with the error, which means no navbar or footer or anything, It doesn't look great.
The PR is getting really huge and quite hard to review, so I would like to propose that once it's ready we merge it in a feature branch, and open a couple more PRs against the feature branch so that we can solve the remaining issues and small details and test everything thoroughly before we deploy the feature. (For example the missing artist page update logic, Brainzplayer at the top level, remaining links that open in a new tab, etc.)
Definitely for another PR, but putting it here for posterity: If i navigate very quickly from one page to the next using the browser navigation, I sometimes hit the API's rate limit.
I think we're gonna want a request manager that will cache API calls accordingly to avoid that (I'm thinking specifically about react-query, it should play well with react-router).
@@ -21,26 +21,26 @@ | |||
</a> |
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.
OK, let's keep those for a follow-up PR, they are not as important
@@ -301,9 +302,9 @@ export default class ListenCard extends React.Component< | |||
} else if (listen.playing_now) { | |||
timeStampForDisplay = ( | |||
<span className="listen-time"> | |||
<a href="/listening-now/" target="_blank" rel="noopener noreferrer"> | |||
<Link to="/listening-now/" target="_blank" rel="noopener noreferrer"> |
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.
This link still opens in a new tab, which I originally thought we would not want. However, I think we want the page to not render the navbar or the footer, which probably means we don't want it to be fully part of the SPA navigation.
In that case it's fine for it to open in a new tab,
…erver into spa-entity-pages
Right now since we're not updating the global context on disabling the external service, we're reloading the page.
This PR migrates the Entity pages and more index pages to SPA