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-1585: Add Syndication Feeds to ListenBrainz #2962

Open
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

ericd23
Copy link
Contributor

@ericd23 ericd23 commented Aug 13, 2024

No description provided.

@mayhem mayhem requested a review from MonkeyDo August 19, 2024 10:47
@mayhem
Copy link
Member

mayhem commented Aug 19, 2024

@MonkeyDo : Can you please have a look at the UI bits of this PR?

@mayhem
Copy link
Member

mayhem commented Aug 19, 2024

@ericd23 : For the URLs that are being used for the feeds, please use the API_URL from config -- on my dev box right now API_URL is set to a localhost address, but the URL goes to listenbrainz.org

@mayhem
Copy link
Member

mayhem commented Aug 19, 2024

Where can I find the UI buttons for the other feeds (I know where the listens button is), but where are the others?

@ericd23
Copy link
Contributor Author

ericd23 commented Aug 19, 2024

Hi mayhem! For fresh releases and stats art, it's on the sidebar. For stats, it's on this page https://listenbrainz.org/user/ercd/stats/top-artists/?range=week.

Copy link
Member

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

UI looks pretty good!
Seems to be aligned with what you discussed with aerozol, although I will leave reviewing the finer details to him as I did not follow all the details.

A few details and comments below, but nothing that will change the UI drastically

frontend/js/src/components/SyndicationFeedModal.tsx Outdated Show resolved Hide resolved
frontend/js/src/explore/art-creator/ArtCreator.tsx Outdated Show resolved Hide resolved
Comment on lines 403 to 406
.feed-button {
cursor: pointer;
color: #353070;
}
Copy link
Member

Choose a reason for hiding this comment

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

This seems unused, and I think you overwrite the text color on the component using style, making it less useful.

I would suggest wrapping the SVG icon inside a parent div element with classes btn btn-icon feed-button.
The btn class handles curser pointer and all that, btn-icon makes a button with just the icon as content, and repurposing the feed-button class to adjust the padding and size:

Suggested change
.feed-button {
cursor: pointer;
color: #353070;
}
.feed-button {
padding: 0;
height: auto;
font-size: 1em;
}

@mayhem
Copy link
Member

mayhem commented Aug 29, 2024

OK, the trailing ? has been fixed and the recommendations feed is fixed -- great. My stats are feed is still empty -- what actions cause new entries to appear in the feed there?

@ericd23
Copy link
Contributor Author

ericd23 commented Aug 29, 2024

Here:
#2962 (comment)
#2962 (comment)

Simply put I embed an API call in the feed. And the base URL for this call is dependent on the env. Since the beta API cannot be used, the feed reader cannot retrieve the SVG. I guess we could temporarily patch the code to use api.listenbrainz.org in the feed content and see if it works ok.

@mayhem
Copy link
Member

mayhem commented Aug 30, 2024

Ah, got it. That makes sense. OK, then I think we're pretty much ready to go merge this, save for one thing: Commenting out the Feed atom feed, since that isn't properly protected right now. Lets hide the feed button in the UI and comment out the API endpoint that corresponds to it. Also, please leave docs as to how to undo the comments once the OAuth stuff is done and rolled out. Thanks!

@ericd23
Copy link
Contributor Author

ericd23 commented Aug 30, 2024

Great!

@ericd23
Copy link
Contributor Author

ericd23 commented Aug 31, 2024

I think there's more to be done than just uncommenting the code once the oauth stuff is done. Once we are there, I will add necessary integration. Just let me know when it's time. I will keep an eye on the chat!

@mayhem
Copy link
Member

mayhem commented Sep 2, 2024

Sounds good -- and we may be getting close to OAuth time -- may not be that long after all. I'm getting last bits of feedback from @Aerozol and @amCap1712, just to make sure we didn't miss anything.

@@ -171,12 +218,14 @@ def get_listens(user_name):

content = render_template(
"atom/listens.html",
user_page_url=_external_url_for("user.index", path="", user_name=user_name),
user_page_url=_external_url_for(
"user.index", path="", user_name=user_name),
user_name=user_name,
recording_mb_page_base_url="https://musicbrainz.org/recording/",
track_name=track_name,
recording_mbid=recording_mbid,
Copy link
Member

Choose a reason for hiding this comment

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

Unable to comment a few lines above so doing it here, I think the listens should fallback to using mapped mbid data if the additional_info doesn't have the required fields? But not a blocker for merging, we can do this later as well.

@mayhem
Copy link
Member

mayhem commented Sep 13, 2024

Hi! Did you see this document from Aerozol?

https://docs.google.com/document/d/10K6JK-9Ytc9lY3cyfjTRyJAmJggaBSNsli2_0-iZ0ks/edit

Its has some UI cleanup that we should do before merging. Do you have time to work on this?

@ericd23
Copy link
Contributor Author

ericd23 commented Sep 14, 2024

Yes I can work on this next week and I will talk to aerozol.

@@ -5,13 +5,19 @@ import { toast } from "react-toastify";
import { Helmet } from "react-helmet";
import { useQuery } from "@tanstack/react-query";
import { useNavigate } from "react-router-dom";
import { FontAwesomeIcon } from "@fortawesome/react-fontawesome";
import { faRss, faSquareRss } from "@fortawesome/free-solid-svg-icons";
Copy link
Member

Choose a reason for hiding this comment

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

faSquareRss seems unused here

});
} else if (pageType === PAGE_TYPE_USER) {
NiceModal.show(SyndicationFeedModal, {
feedTitle: `User-specific Fresh Releases`,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be customized?

`${currentUser.name}'s Fresh Releases`

@MonkeyDo MonkeyDo requested a review from mayhem October 21, 2024 11:23
@mayhem
Copy link
Member

mayhem commented Nov 13, 2024

The following UI improvements are still needed before release:

https://tickets.metabrainz.org/browse/LB-1678

The title on this page is different from the header-with-line wwe use on other pages, making the placement of an rss feed like other pages more difficult.
Changed the playlist title and RSS button to follow other pages and unify the overall look.
and allowing us to plunk an RSS feed button in there at the same place everytime for consistency
a short name on hover
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants