fix: spacing in artist pages, translation and cards #645
Conversation
|
I can count six different left margins in a single photo. Even the previous state had five, although the bottom padding on the title is a welcome change. |
|
The lack of alignment between the buttons and the cover is annoying :/ I could see the card only having the Play button, but being larger, then have the two small icons aligned to the bottom of the card.... but then the card is big enough to have the buttons on it directly. Either way, the card needs to be bigger 😋 |
ferferga
force-pushed
the
artists-improvements
branch
from
8b99059
to
84b3a5a
Compare
|
Page still needs a huge responsive pass, but after your comments I also tried to keep this somewhat responsive: Desktop
Mobile |
We should probably merge #616 before this, then, so we can adjust it properly with the few changes I made in there (I adjusted album title size based on size, among other things) |
|
@ferferga I'd left align everything under the "Albums" label at the same level as it :) |
ferferga
force-pushed
the
artists-improvements
branch
from
84b3a5a
to
bc47e36
Compare
|
Final design: Mobile (As in Spotify, in mobile design the tracklist isn't shown. One can see the tracklist by clicking on an specific album)
Desktop
|
ferferga
force-pushed
the
artists-improvements
branch
from
bc47e36
to
9db799b
Compare
Codecov Report
@@ Coverage Diff @@
## master #645 +/- ##
=======================================
Coverage 11.99% 11.99%
=======================================
Files 134 134
Lines 3534 3534
Branches 527 527
=======================================
Hits 424 424
Misses 3087 3087
Partials 23 23
Continue to review full report at Codecov.
|
ferferga
force-pushed
the
artists-improvements
branch
2 times, most recently
from
49b498f
to
0491004
Compare
|
Wouldn't it be better (like spotify) to have a grid of album covers, at the moment they look a bit too big imo. |
|
Would also like to have a means of sorting these albums by year, or better yet, have an ascending sort by year as a default! |
|
Actually, It looks like Spotify doesn't do that. What I was thinking of was using the item Grid on mobile devices, that way we could fit 2/3 widthwise. |
|
@camc314 Can you post an screenshot of what you mean please? For reference, this is the latest version of Spotify in Android, I don't know if in iOS it's different as I don't use any iOS device, but I'm aware there are some subtle differences in the way the layout is organised. |
|
Spotify doesn't actually do what I thought for artist pages. I though it had an area like this
It's just a thought, I don't mind what we decide to do |
| @@ -120,10 +120,10 @@ export default Vue.extend({ | |||
| return false; | |||
| } | |||
| }, | |||
| overlay: { | |||
| noOverlay: { | |||
There was a problem hiding this comment.
I would prefer we leave it as overlay and set the default to false, although I can do that in a separate PR
There was a problem hiding this comment.
We're switching most of the props (see #592 (comment))
There was a problem hiding this comment.
I don't quite understand, The comment, on 592 is what I'm saying? Let me know if that's wrong.
There was a problem hiding this comment.
MrTimscampi says that, if a property is not specified, it's false by default. Before, we were defaulting overlay to true and, for disabling it, we needed to pass it like :overlay="false", which is counter-intuitive.
Other props in card also follow the approach this prop will follow now: they are like noX and setting them means they're false, no need to do the :overlay="false" thing anymore, just no-overlay when using the component.
There was a problem hiding this comment.
Ah I didn't notice that all the others were noX, imho, like vuetify does, we should prefer the other method (instead of noOverlay have overlay). Since we do this elsewhere we can leave for now. Thoughts @ThibaultNocchi
There was a problem hiding this comment.
I think Cam is just saying to keep the property as overlay even though it is at false by default (which I agree should be)
Edit: yeah what you said Cam ^^
There was a problem hiding this comment.
So we will leave as is for now, and refactor later. @ThibaultNocchi one last approval before merge?
|
@camc314 Oh I see what you mean now. That one isn't bad either. We already have the same layout in the library browsing pages and this makes me push towards the "row" design so there's a bit of differentiation. I don't mind either though, both are great ways to present the content tbh. Thoughts @ThibaultNocchi ? |
That's cool with a small library but isn't exactly practical with a large one. This forces a ton of scrolling to see any relevant results for artists you have more than one album for. For example, I have something like 50 Elvis albums. The most recent is a 2019 compilation of his 1967 concerts, which has 8 discs. I'm looking for a 1950 album. That means I'm scrolling for quite some time before I finally get there. With most other streamers, the albums are presented in a horizontal scroller with the tracks obviously not expanded, so there's far less work involved. Since the excessive vertical scrolling is forced here, having the ability to sort would at least allow you to choose from wihch end you want to start scrolling. |
|
@ferferga Yeah I like Cam's idea, having a grid of covers with only their name and year could be great. We don't have to duplicate the track listing as the album is only a click away, and we can still add a whole album to the queue in a click. I also agree with neopc's idea about being able to sort them on the artist page, but I'd leave that as an idea for later and just get this first view of the artist page. |
|
Shall we leave it as is for now, and merge, and then refactor over the next few weeks to use the grid? |
ferferga
force-pushed
the
artists-improvements
branch
from
0491004
to
f924ab2
Compare
|
I like both the grid view and a list view. Maybe an option in settings in the future? But I like it either way |
ferferga
force-pushed
the
artists-improvements
branch
from
f924ab2
to
8a9dcae
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
LGTM, but we'll really need to do the responsive pass on this page :p |
Spotify
Vue before
Vue after
