-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
@@ -0,0 +1,27 @@ | |||
.bulbs-truncate-element-parent-active { |
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.
It works great. Maybe we can add a transition here? Would be a little nicer if we had a slight animation on the height of the active class or opacity attribute of the :after element. Maybe both?
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.
Leaving off the animation because it's not quick and easy to animate between max-height:1000px
and max-height:auto
. We can come back to this after QA has a look at the functionality.
Other than small comment. lgtm |
} | ||
} | ||
|
||
.bulbs-truncate-element-button { |
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.
could add outline: none
here to prevent that annoying blue border when clicking
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.
Nice element :)
this.openElement = this.openElement.bind(this); | ||
this.addButton(); | ||
this.addEventListener('click', this.openElement); | ||
$(this).prev().addClass('bulbs-truncate-element-parent-active'); |
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 use of native dom methods in this element, another two apis we can use here
this.previousElementSibling
and element.classList
Together we can use them like:
let previousEl = this.previousElementSibling;
if (previousEl) {
previousEl.classList.add('bulbs-truncate-element-parent-active');
previousEl.classList.remove('bulbs-truncate-element-parent-active');
}
Doing this now will help when we start migrating these components over to the gawkermedia/kinja-components
repository which does not include jQuery as a depency.
Working PR for the article truncation
Test link:
http://avc-refresh-truncate-article.test.avclub.com/article/hear-rare-track-quiet-melancholy-los-campesinos-250252