Closed Bug 936397 Opened 6 years ago Closed 5 years ago

[music2] [2] Implement the current views with tablet style

Categories

(Firefox OS Graveyard :: Gaia::Music, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED INVALID
1.3 Sprint 5 - 11/22

People

(Reporter: dkuo, Assigned: dkuo)

References

Details

Attachments

(1 file)

46 bytes, text/x-github-pull-request
squib
: review+
eshapiro
: feedback+
Details | Review
After the first item(Bug 936385) of music2 is landed, we can start to implement the features that the current music has, if the schedule is allowed, probably we can implement the features with phone style as well.

This item should cover:
2-1. Mix.
2-2. Playlists.
2-3. Albums, Artists and All Songs.
Summary: [music2] [2] Implement the current music features with tablet style → [music2] [2] Implement the current views with tablet style
The first part of bug 936385 is landed, this bug is not blocked so taking it and start to implement it.
Assignee: nobody → dkuo
this is subset of Music2 re-layout for tablet, tag it 1.3+ with target milestones.
blocking-b2g: --- → 1.3+
Whiteboard: [Flatfish only]
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Whiteboard: [Flatfish only] → [Flatfish only][developer+]
Blocks: 936405
blocking-b2g: 1.3+ → ---
Attached file Part 1 for 2-2 and 2-3
Jim,

This is the first patch for this bug, I have implemented the playlists, albums, artists and songs views but mix view because, the UX design of mix view is still under discussing. And basically I followed Evan's code structure and style so it should be not too hard to understand.
And there are two new js that I imported from shared/js, which are screen_layout.js and template.js that you probably not familiar with. Simply say, screen_layout.js is for judging the screen size, and template.js is for easier to render the DOM elements. Would you please review this one? thanks.

Evan,

I am not sure if you are busy for school so I set feedback to you but review so that I can get Jim's review and your feedback as well, and feel free to give us any ideas if you like, thanks.
Attachment #8336242 - Flags: review?(squibblyflabbetydoo)
Attachment #8336242 - Flags: feedback?(eshapiro)
Comment on attachment 8336242 [details] [review]
Part 1 for 2-2 and 2-3

I believe this was an issue prior to the patch, but on desktop, I'm seeing an issue where the mix page isn't deactivated when you click on another page, and so you get both the mix view and, say, the playlists view at once. If it's not too hard to fix, I think we should try to fix that here, since it'll make things work a lot better.

Otherwise, the code seems good, but I'm going to do another pass before I finish my review. I just wanted to note this issue now before I forget.
(In reply to Jim Porter (:squib) from comment #4)
> Comment on attachment 8336242 [details] [review]
> Part 1 for 2-2 and 2-3
> 
> I believe this was an issue prior to the patch, but on desktop, I'm seeing
> an issue where the mix page isn't deactivated when you click on another
> page, and so you get both the mix view and, say, the playlists view at once.
> If it's not too hard to fix, I think we should try to fix that here, since
> it'll make things work a lot better.

No problem, I will see where is the issue and you can keep on reviewing, thanks.
Jim, I have addressed the issue you mentioned in comment 4, that was an easy fix, it seems like a gecko issue but I am not sure. That is, if music2 is launched by its url with "#select-mix", then we select the other pages like you said, both of the :target css role will be applied to mix view and the view we select, and that's why the mix page was still activated. So I just added window.location.hash = '#select-mix' to tirgger the hashchange, it should be easy for other people to understand.
Comment on attachment 8336242 [details] [review]
Part 1 for 2-2 and 2-3

I've looked over this more and I think this is good to land. There might be things we want to clean up later, but this is a huge step towards making music2 usable, and I think we should land it as soon as possible so more people can try it out.
Attachment #8336242 - Flags: review?(squibblyflabbetydoo) → review+
I haven't had a chance to run it, but I just looked through the code, lgtm! I agree about landing it sooner rather than later - having somewhat of a foundation in place seems like a good idea for moving forward.
Attachment #8336242 - Flags: feedback?(eshapiro) → feedback+
Thanks guys! the travis finally gets green.

2-2 and 2-3 landed on master: bb5b11b420901fb2a6f0ffab5e6fb128e1d6160e
Whiteboard: [Flatfish only][developer+] → [flatfish][TCP]
Whiteboard: [flatfish][TCP]
Closing all music2-specific bugs. The current plan is to gradually incorporate bits of music2 into the current music app, since it will take too long to achieve feature parity in music2.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.