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

Bug 1263840 - [TV][Home] Move card description text to outside of but… #34240

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@rexboy7
Copy link
Contributor

commented Apr 14, 2016

…tons

@rexboy7 rexboy7 force-pushed the rexboy7:bug-1263840 branch from 87cf208 to 216d96f Apr 14, 2016

.card-picker-panel {
top: 80%;
position: absolute;
}

card-picker-grid-view

This comment has been minimized.

Copy link
@luke-chang

luke-chang Apr 15, 2016

Contributor

???

This comment has been minimized.

Copy link
@rexboy7

rexboy7 Apr 18, 2016

Author Contributor

my bad :-/

@@ -37,7 +37,8 @@
});
this._keyNavigationAdapter.on('enter-keyup', this.onEnter.bind(this));

this._selectedElements = this.gridView.getElementsByClassName('selected');
this._selectedButtons = this.gridView.getElementsByClassName('selected');
this._appButtons = this.gridView.getElementsByTagName('smart-button');

this.refresh();

This comment has been minimized.

Copy link
@luke-chang

luke-chang Apr 15, 2016

Contributor

Not related to this bug, but ... Do we need to call refresh in the init stage? I suppose it'll be called again when it's about to show, right?

This comment has been minimized.

Copy link
@rexboy7

rexboy7 Apr 18, 2016

Author Contributor

Yeah, I think you're right.

this.appButtons.concat(this.navigableElements));
this._spatialNavigator.focus(this.appButtons[0]);
this.navigableElements.concat(Array.from(this.allItems)));
this._spatialNavigator.focus(this.allItems[0]);

This comment has been minimized.

Copy link
@luke-chang

luke-chang Apr 15, 2016

Contributor

nit: You can simply call focus() without arguments if you intend to focus the first element in the collection.

This comment has been minimized.

Copy link
@rexboy7

rexboy7 Apr 18, 2016

Author Contributor

I think allItems[0] is not the first item of collection here (but in old version it is). But I can change it back to the old version and remove the parameter if you think that's better.

This comment has been minimized.

Copy link
@luke-chang

luke-chang Apr 18, 2016

Contributor

Oh, it's my fault! You don't need to change it here. Sorry about that.

var appCardElem = createCardElemHelper(card);
if (appCardElem) {
var cardButton = appCardElem.firstElementChild;
cardButton.dataset.parentType = 'folder';

This comment has been minimized.

Copy link
@luke-chang

luke-chang Apr 15, 2016

Contributor

How about put dataset.parentType in createCardElemHelper if it's a required property?

This comment has been minimized.

Copy link
@rexboy7

rexboy7 Apr 18, 2016

Author Contributor

For now parentType is used only in updateFolder to determine if we should move it out of folder (if it was a folder's card), put it into a folder (if it was a main list's card), or do nothing. This is not used elsewhere since there's no other usecases that need it.

This comment has been minimized.

Copy link
@luke-chang

luke-chang Apr 18, 2016

Contributor

Actually, I meant you can define function createCardElemHelper(card, parentType), then pass createCardElemHelper(card, 'folder') here and pass createCardElemHelper(card, 'empty') below. However, it's up to you. :-p

@@ -305,7 +305,8 @@
onCardUpdated: function(card, idx) {
var cardButton = this.cardScrollable.getItemFromNode(
this.cardScrollable.getNode(idx));
CardUtil.updateCardName(cardButton, card);
// nextElementSibling of cardButton is the name span.
CardUtil.updateCardName(cardButton.nextElementSibling, card);

This comment has been minimized.

Copy link
@luke-chang

luke-chang Apr 15, 2016

Contributor

Why not retrieve the name span by something like this.cardScrollable.getNode(idx).querySelector('.name')? It would be more readable than nextElementSibling. (since we don't really need cardButton here.)

This comment has been minimized.

Copy link
@rexboy7

rexboy7 Apr 18, 2016

Author Contributor

Agree. Let's change it.

}
});
updateCardName: function(nameElement, card) {
if (nameElement.classList.contains('name')) {

This comment has been minimized.

Copy link
@luke-chang

luke-chang Apr 15, 2016

Contributor

If everyone call this API correctly, I suppose we don't need to verify its classList. (we may just need to check if it's null.)

This comment has been minimized.

Copy link
@rexboy7

rexboy7 Apr 18, 2016

Author Contributor

I don't completely understand the old code's logic so I just kept that. But I can remove it and go back if we find some isuse.

This comment has been minimized.

Copy link
@luke-chang

luke-chang Apr 18, 2016

Contributor

I guess the original logic behind the spans depends on SharedUtils.nodeListToArray so we need to filter that. However, since this function is only called by home.js now, we should be able to make sure nameElement is always valid (or null, maybe).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.