Add and apply album art to pages. #2615

Merged
merged 5 commits into from Jul 20, 2012

Projects

None yet

2 participants

@dominickuo
Collaborator

No description provided.

@vingtetun vingtetun and 1 other commented on an outdated diff Jul 19, 2012
apps/music/style/music.css
+
+.album-contorls-button {
+ background: -moz-linear-gradient(center top , #00ADEE, #0078A5) repeat scroll 0 0 transparent;
+ border: 1px solid #0076A3;
+ color: #D9EEF7;
+}
+.album-contorls-button:hover {
+ background: -moz-linear-gradient(center top , #0095CC, #00678E) repeat scroll 0 0 transparent;
+}
+.album-contorls-button:active {
+ background: -moz-linear-gradient(center top , #0078A5, #00ADEE) repeat scroll 0 0 transparent;
+ color: #80BED6;
+}
+.album-contorls-button:disabled {
+ opacity: 0.3;
+}
@vingtetun
vingtetun Jul 19, 2012 collaborator

Add line breaks between your block of rules.

@dominickuo
dominickuo Jul 20, 2012 collaborator

OK, done.

@vingtetun vingtetun commented on the diff Jul 19, 2012
apps/music/index.html
@@ -32,7 +32,7 @@
<script type="text/javascript" src="js/music.js"></script>
</head>
- <body class="hidden">
+ <body class="invisible">
@vingtetun
vingtetun Jul 19, 2012 collaborator

Why do you switch from hidden -> invisible.

@dominickuo
dominickuo Jul 20, 2012 collaborator

The two classes look like this:

.hidden { display: none; }

.invisible { visibility: hidden; }

Actually, I used different properties in .hidden and .invisible.
The reason I used visibility: hidden instead of display: none is because,
if the body is set to display: none, the styles of body(and its children) will be all 0 or null.
At this time, I need some styles of the elements in the body, it will cause bugs due to the styles are all 0 or null.
So I used visibility: hidden which has the same vision as display: none,
and I can still get the styles at this time.

@vingtetun vingtetun and 1 other commented on an outdated diff Jul 19, 2012
apps/music/index.html
@@ -58,10 +58,24 @@
<div class="sub-tile"><br></div>
</div>
<div id="views-list" class="view"></div>
- <div id="views-sublist" class="view"></div>
+ <div id="views-sublist" class="view">
+ <div id="views-sublist-header">
+ <div id="views-sublist-header-album">
+ <div id="views-sublist-header-default-image">&#9834;</div>
+ <img id="views-sublist-header-image" />
+ </div>
+ <div id="views-sublist-header-contorls">
+ <div id="views-sublist-header-name">Name</div>
+ <button id="views-sublist-contorls-play" class="album-contorls-button" disabled>Play All</button>
+ <button id="views-sublist-contorls-shuffle" class="album-contorls-button" disabled>Shuffle</button>
@vingtetun
vingtetun Jul 19, 2012 collaborator

Maybe the ids are a bit long :)

@vingtetun
vingtetun Jul 19, 2012 collaborator

Oh... and there is a typo contorls -> controls which is used in the css too...

@dominickuo
dominickuo Jul 20, 2012 collaborator

I corrected the typo, and modified "views-sublist-header-default-image" to "views-sublist-header-default"

@vingtetun vingtetun and 1 other commented on an outdated diff Jul 19, 2012
apps/music/js/MetadataParser.js
callback(metadata);
- }, {
+ }, {tags: ['album', 'artist', 'title', 'picture'],
dataReader: FileAPIReader(file)
});
@vingtetun
vingtetun Jul 19, 2012 collaborator
  {
    tags: ['album', 'artist', 'title', 'picture'],
    dataReader: FileAPIReader(file)
  });
@dominickuo
dominickuo Jul 20, 2012 collaborator

Done.

@vingtetun vingtetun and 1 other commented on an outdated diff Jul 19, 2012
apps/music/js/music.js
this.view.appendChild(li);
+
+ // Set source to image and crop it to be fitted when it's onloded
+ var image = result.metadata.picture;
+ if (image) {
+ img.onload = function(evt) {
+ cropImage(evt);
+ }.bind(this);
@vingtetun
vingtetun Jul 19, 2012 collaborator

You don't need bind.

@dominickuo
dominickuo Jul 20, 2012 collaborator

Yap, : P
And I added a function called setItemImage to ListView.

@vingtetun vingtetun and 1 other commented on an outdated diff Jul 19, 2012
apps/music/js/music.js
this.view.scrollTop = 0;
},
+ setAlbumSrc: function slv_setAlbumSrc(url) {
+ // Set source to image and crop it to be fitted when it's onloded
+ this.albumImage.src = url;
+ this.albumImage.classList.remove('fadeIn');
+
+ this.albumImage.onload = function(evt) {
+ cropImage(evt);
+ this.albumImage.classList.add('fadeIn');
+ }.bind(this);
@vingtetun
vingtetun Jul 19, 2012 collaborator

Use addEventListener instead of onload directly.

@dominickuo
dominickuo Jul 20, 2012 collaborator

Done.

@vingtetun vingtetun commented on the diff Jul 19, 2012
apps/music/js/music.js
- this.index++;
@vingtetun
vingtetun Jul 19, 2012 collaborator

Why don't you keep this line since you want to do it at the end and you have to do a this.index+1 a few lines after.

@dominickuo
dominickuo Jul 20, 2012 collaborator

The reason I put this.index++; at the end of the SubListView.update() is because I wished all the operations in this function can be done, then the index increases.
Although I can use index after this.index++; when I need this.index+1, but this may confuse reviewers and me cause the index is changed before SubListView.update() in finished.

@vingtetun vingtetun and 1 other commented on an outdated diff Jul 19, 2012
apps/music/js/music.js
@@ -450,6 +535,22 @@ var PlayerView = {
songData.metadata.album : navigator.mozL10n.get('unknownAlbum');
this.currentIndex = targetIndex;
+ // Reset the image to be ready for fade-in
+ this.coverImage.src = '';
+ this.coverImage.classList.remove('fadeIn');
+
+ // Set source to image and crop it to be fitted when it's onloded
+ var image = songData.metadata.picture;
+ if (image) {
+ this.coverImage.onload = function(evt) {
+ cropImage(evt);
+
+ this.coverImage.classList.add('fadeIn');
+ }.bind(this);
@vingtetun
vingtetun Jul 19, 2012 collaborator

Use addEventListener instead of onload.

@dominickuo
dominickuo Jul 20, 2012 collaborator

Done.

@vingtetun vingtetun commented on the diff Jul 19, 2012
apps/music/js/music.js
@@ -699,5 +804,5 @@ window.addEventListener('localized', function showBody() {
document.documentElement.dir = navigator.mozL10n.language.direction;
// <body> children are hidden until the UI is translated
- document.body.classList.remove('hidden');
+ document.body.classList.remove('invisible');
@vingtetun
vingtetun Jul 19, 2012 collaborator

This is consistent in all the other file (i don't like that but that's how it is done for the moment). Please keep hidden instead of invisible.

@dominickuo
dominickuo Jul 20, 2012 collaborator

I have explained why I changed "hidden" to "invisible" in the previous comment.
If this is still an issue to you, I can change back to "hidden";

@vingtetun vingtetun commented on the diff Jul 19, 2012
apps/music/js/utils.js
@@ -32,3 +32,34 @@ function formatTime(secs) {
return formatedTime;
}
+
+// This function is for cropping the onloaded IMG.
+// If a DIV(parent) contains a IMG(child),
+// the IMG will be cropped and fitted to vertical or horizontal center.
+// DIV(parent) - overflow: hidden;
+// IMG(child) - position: absolute;
+function cropImage(evt) {
+ var image = evt.target;
+
+ var parentWidth = image.parentElement.clientWidth;
+ var parentHeight = image.parentElement.clientHeight;
+
+ var childRatio = image.naturalWidth / image.naturalHeight;
+ var parentRatio = parentWidth / parentHeight;
+
@vingtetun
vingtetun Jul 19, 2012 collaborator

var style = image.style;

@dominickuo
dominickuo Jul 20, 2012 collaborator

Done.

@vingtetun vingtetun merged commit 067bcfc into mozilla-b2g:master Jul 20, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment