Create CollapseButton component class to standardize appearance of this button. #11462
Conversation
cjcenizal
added
Team:Platform-Design
cjcenizal
force-pushed
the
11459/bug/caret-buttons
branch
from
6e8f2ae
to
9b5a374
Compare
cjcenizal
force-pushed
the
11459/bug/caret-buttons
branch
from
9b5a374
to
643206e
Compare
There was a problem hiding this comment.
Hmm, ideally, if we're modifying this directive, we would do the following:
- Make the directive an attribute or element directive rather than a class directive (probably making use of
transcludeor something to make sure we don't modify the content) - Move the HTML into a template file and use something like
ng-classto control the class (instead of manually adding/removing classes whenever clicked)
Thoughts @cjcenizal? I understand this would take a bit more time to do, but I think it might be worth it in the end, since you're already modifying this directive.
|
@lukasolson You're right. I thought about it when I was working on this, too. But I think our sidebar will undergo some design and refactoring changes in the future, so I was thinking it'd be best to make the minimally invasive change now, and let this code get swept away during redesign or refactoring phases in the future. Also, in terms of benefits, how much maintenance will this code require? If not that much, then will refactoring it now hold much benefit for people in the future? |
cjcenizal
force-pushed
the
11459/bug/caret-buttons
branch
from
643206e
to
7a39e8d
Compare
| @@ -23,6 +32,17 @@ uiModules | |||
|
|
|||
| $collapser.on('click', function () { | |||
| $elem.toggleClass('closed'); | |||
There was a problem hiding this comment.
If you're moving to a add/remove class instead of toggle class, why not move this down as well?
|
@lukasolson Good point man! Addressed. Could you take another look? |
cjcenizal
force-pushed
the
11459/bug/caret-buttons
branch
from
0edadb9
to
b72bb84
Compare
|
jenkins, test this |
…test subject selector.
cjcenizal
force-pushed
the
11459/bug/caret-buttons
branch
from
b72bb84
to
9bfe603
Compare
|
Is this the intended behavior?
Seems a little funky that the collapse button would stay in that same spot. I believe before it would go to the top (and not cover the content). |
|
I guess I was wrong... It looks like it used to be that way too. Ignore that last comment. 😄 |
…is button. (elastic#11462) * Create CollapseButton component class to standardize appearance of this button. * Fix positioning of LocalSearch icon. * Update collapsible-sidebar directive and Discover page object to use test subject selector. * Refactor 'closed' class assignment.
Addresses #11459
Notice the 4th caret, in the top-right corner of the timepicker dropdown.
Before
After