Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

Bug 1095826 - [RTL][Video] Thumbnail images do not appear #26456

Merged
merged 1 commit into from Dec 12, 2014

Conversation

russnicoletti
Copy link

No description provided.

@try-server-hook
Copy link

russnicoletti russnicoletti started tests. Results

@@ -89,6 +89,18 @@ li .details {
right: 0;
overflow: hidden;
}

/* In RTL languages, image and details are mirrored */
*[dir=rtl] li .details {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that this would be more efficent CSS if you used "HTML[dir=rtl]" instead of "*[dir=rtl]"

background: url(images/unwatched.png) no-repeat center / 100%;
position: absolute;
bottom: 0;
width: 0.4rem;
top: 0;
order: 1;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, the order would not be necessary and it would just be specified by the HTML. But I think currently the unwatched thing is second in the html instead of first.

@try-server-hook
Copy link

russnicoletti russnicoletti started tests. Results

@@ -88,7 +96,10 @@ li .details {
top: 0;
right: 0;
overflow: hidden;
order: 3;
-moz-margin-start: 22rem;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that with the flexbox, you shouldn't have to use absolute positioning and a really wide margin and a width:calc(). Ideally, you'd just use the flex: property to make this a flexible child so that it fills up the available space. (Having said that, however, I'll confess that I have not used flexboxes myself yet)

@try-server-hook
Copy link

russnicoletti russnicoletti started tests. Results

}
.unwatched {

li .inner .unwatched {
background: url(images/unwatched.png) no-repeat center / 100%;
position: absolute;
bottom: 0;
width: 0.4rem;
top: 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess you no longer need bottom and top declarations here now that you have align-items: stretch

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to my testing, they do matter. They unwatched icon/image does not appear without the top and bottom properties.

@try-server-hook
Copy link

russnicoletti russnicoletti started tests. Results

display: flex;
flex-direction: row;
justify-content: flex-start;
align-items: stretch;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have much experience with display:flex, but it seems to me that you are not making much use of it. The align-items:stretch may actually be doing something helpful here, but you might actually be able to just do display:inline and have it work as well. Since you're using calc() in the .details element instead of making that flexible I'm not sure you need this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that display:inline doesn't provide the desired behavior (the image doesn't appear, the 'details' take up the entire screen). display:flex or display:inline-flex provide the desired behavior.

russnicoletti pushed a commit that referenced this pull request Dec 12, 2014
Bug 1095826 - [RTL][Video] Thumbnail images do not appear r=djf
@russnicoletti russnicoletti merged commit 86d9509 into mozilla-b2g:master Dec 12, 2014
@russnicoletti russnicoletti deleted the bug-1095826 branch September 23, 2015 18:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants