-
-
Notifications
You must be signed in to change notification settings - Fork 312
feat: New MusicBrainz Homepage #3669
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for opening your first pull request for MusicBrainz Server, and welcome to our community! 🎉 |
reosarevok
left a comment
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 haven't gotten to the scripts folder yet, so more coming tomorrow probably for that, but putting up the comments I have for now since there's... a fair bunch. A lot are just trivial indent fixes though :)
| WHERE event_art_type.type_id = ? | ||
| AND event_art.ordering = 1 | ||
| AND edit.type = ? | ||
| AND event_art.date_uploaded < NOW() - INTERVAL '10 minutes' |
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.
Why limit (this much, at least) the art upload date if what we care is to see the most current events? Wouldn't it be better to just show the ones for the recent past and maybe the upcoming week that have artwork if any? Same for fresh releases.
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.
It looks like this line is just ensuring the uploads are at least 10 minutes old, which was copied from 6add14f.
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.
Oh. Duh. I should read properly.
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.
A small issue I do see with this query is that events displayed here seem guaranteed to be the same for quite a while if someone adds a tour a few months in the future. So I still think (even if it's unrelated to this specific line) having some sort of "max X days in the future" makes sense to show stuff here too. What do you feel?
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.
That makes sense to me. I'd probably keep it short, like max 3 days into the future.
reosarevok
left a comment
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 haven't gotten to the scripts folder yet, so more coming tomorrow probably for that, but putting up the comments I have for now since there's... a fair bunch. A lot are just trivial indent fixes though :)
|
The browser console is being spammed with some swiper-related warning. Can you look into that too? :)
|
reosarevok
left a comment
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 just noticed this change is entirely dropping the admin menu. Please make sure it's available somewhere, either on the top as now or (since I guess it doesn't fit well there) at the very least on the "Editor tools" section.
Most if not all the /homepage files seem like they should be named in UpperCamelCase, since they are components (MobileSearchPopup and whatnot).
reosarevok
left a comment
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.
Last few comments (I did not check the less files nor the tests at the moment).
Sorry about some duplicate comments being sent, I thought I lost them at first because of github hiding comments for huge PRs.
ddbbfe4 to
375f7ce
Compare
ad843a4 to
4f8c861
Compare
String locations are needed when building translations for the client-side JS (which uses msggrep). This should prevent, e.g., hydration errors in cases where a string has been moved.
…omponent type definitions
`l` was being incorrectly used on dynamic strings that weren't previously marked with `N_l` within the same file.
* `lp` is unused. * `l` is auto-imported by the ProvidePlugin.
`l` was being incorrectly used on dynamic strings that weren't previously marked with `N_l` within the same file.
Co-authored-by: Nicolás Tamargo <[email protected]>
4f8c861 to
a62f676
Compare
…itor tools with cookie-based state management
…rver into ansh/new-homepage
…those within the last 7 days and the next 3 days
Problem
🔰 Reference related tickets with MBS-XXX at least.
Solution
🔰 If you have a lot to say, be more detailed in this section.
Testing
🔰 If you tested anything on specific pages, mention them here, such as:
Tested something on
/release/mbidusing a sample/full databaseDocumenting
🔰 If you updated documentation pages, mention them here, such as:
Updated WikiDocs page
Draft progress
Further action