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
Lightbox 2.0: be able to expand overflown caption #10105
Conversation
20de636
to
6b37713
Compare
6b37713
to
f3f7941
Compare
opacity: 1; | ||
transition: opacity 1s; | ||
} | ||
|
||
.i-amphtml-lbv-desc-box.standard { | ||
max-height: 100px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using a unit tied to font-size will be better here. Maybe something around 7rem;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but I think using em
could also be fine? It's the size of the description box after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should avoid em
since it is relative to ancestor's font-size. Given lightbox-viewer is always child of body, it does not matter much in this case but rem
is preferred.
* @private | ||
*/ | ||
toggleDescriptionOverflow_() { | ||
if (this.descriptionBox_.classList.contains('standard')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use vsyn and do the scrollHeight
clientHeight
reads in the measure block and then mutate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
opacity: 1; | ||
transition: opacity 1s; | ||
} | ||
|
||
.i-amphtml-lbv-desc-box.standard { | ||
max-height: 6em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do rem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok~
if (this.descriptionBox_.classList.contains('standard')) { | ||
this.descriptionBox_.classList.remove('standard'); | ||
this.descriptionBox_.classList.add('overflow'); | ||
let descBoxHeight = -1, descTextAreaHeight = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use the state
provided by vsyn to hold there values
measure: state => {
state.foo = 'bar';
},
mutate: state => {
state.foo
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, done
Partial for #9970, more about transition coming in another PR