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

Single page application feature branch #2851

Merged
merged 123 commits into from Apr 26, 2024
Merged

Single page application feature branch #2851

merged 123 commits into from Apr 26, 2024

Conversation

MonkeyDo
Copy link
Contributor

@MonkeyDo MonkeyDo commented Apr 25, 2024

Continuing from #2731, except this time we split the work into a few PRs, and reviewed and merged them into a feature branch.
The components of this PR have been reviewed and tested manually, and this PR is mainly to have a paper trail.

PRs in this feature branch:

#2769
#2822

anshg1214 and others added 15 commits April 24, 2024 17:23
Otherwise, ToastContainer and NiceModal both won't have access to any of the react-router context, which breaks if you use a Link component for example, or want a 'refetch' button in a toast messsage.
Noiw that we navigate with react-router, the modal backdrop ends up staying open after navigation, making the page pretty much useless.

So close the modal on click, and also ensure when you click the backdrop it closes the modal
Otherwise our "back" buttons at the top of the page take you back to the redirect and back to the album page you just came from
This is so the secondary navbar can span the whole width rather than be constrained to a max width; since this is what we want for the rest of the content, we have applied the role="main" attribute everywhere and we can use that.
Copy link
Contributor Author

@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.

Found some minimal stuff, fixed what I could directly, but nothing that prevents us from moving forward 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't tested this page at all, I didn't even know it existed!
It's for the old (deprecated?) LastFM API compatibility.
Will see how to try it out

</div>
<br />
<div className="well" style={{ maxWidth: "600px", margin: "0 auto" }}>
<a
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checking that we do indeed want an anchor tag here and not a Link. Makes sense, but I thought I'd check since the login is pretty crucial and we don't often use it ourselves


describe("ListensControls", () => {
// eslint-disable-next-line jest/no-disabled-tests
xdescribe("ListensControls", () => {
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 have to revisit this, with #2844 and the HueSound refactor I've learned a lot about how to test this stuff, but it's going to take me a bit of time

frontend/js/tests/test-utils/rtl-test-utils.tsx Outdated Show resolved Hide resolved
anshg1214 and others added 12 commits April 25, 2024 19:08
This prevents the navbar and the footer from re-rendering on switching between protected and public routes.
This is used with CSS to limit the width of the main page content
BrowserRouter goes first so that everything inside (such as toast messages) can have access to react-router context
@MonkeyDo MonkeyDo merged commit 83eef55 into master Apr 26, 2024
2 of 3 checks passed
@MonkeyDo MonkeyDo deleted the single-page-app branch April 26, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants