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

Create CollapseButton component class to standardize appearance of this button. #11462

Merged
merged 5 commits into from May 4, 2017

Conversation

Projects
None yet
3 participants
@cjcenizal
Contributor

cjcenizal commented Apr 27, 2017

Addresses #11459

Notice the 4th caret, in the top-right corner of the timepicker dropdown.

Before

image

After

image

@lukasolson

Hmm, ideally, if we're modifying this directive, we would do the following:

  1. Make the directive an attribute or element directive rather than a class directive (probably making use of transclude or something to make sure we don't modify the content)
  2. Move the HTML into a template file and use something like ng-class to 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.

@cjcenizal

This comment has been minimized.

Show comment
Hide comment
@cjcenizal

cjcenizal Apr 28, 2017

Contributor

@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?

Contributor

cjcenizal commented Apr 28, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@cjcenizal

cjcenizal May 2, 2017

Contributor

@lukasolson Good point man! Addressed. Could you take another look?

Contributor

cjcenizal commented May 2, 2017

@lukasolson Good point man! Addressed. Could you take another look?

@cjcenizal

This comment has been minimized.

Show comment
Hide comment
@cjcenizal

cjcenizal May 3, 2017

Contributor

jenkins, test this

Contributor

cjcenizal commented May 3, 2017

jenkins, test this

@lukasolson

This comment has been minimized.

Show comment
Hide comment
@lukasolson

lukasolson May 4, 2017

Member

Is this the intended behavior?

may-04-2017 13-12-49

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).

Member

lukasolson commented May 4, 2017

Is this the intended behavior?

may-04-2017 13-12-49

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).

@lukasolson

This comment has been minimized.

Show comment
Hide comment
@lukasolson

lukasolson May 4, 2017

Member

I guess I was wrong... It looks like it used to be that way too. Ignore that last comment. 😄

Member

lukasolson commented May 4, 2017

I guess I was wrong... It looks like it used to be that way too. Ignore that last comment. 😄

@lukasolson

LGTM!

@cjcenizal cjcenizal merged commit e91c99f into elastic:master May 4, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

@cjcenizal cjcenizal deleted the cjcenizal:11459/bug/caret-buttons branch May 4, 2017

cjcenizal added a commit to cjcenizal/kibana that referenced this pull request May 4, 2017

Create CollapseButton component class to standardize appearance of th…
…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.

cjcenizal added a commit that referenced this pull request May 4, 2017

Create CollapseButton component class to standardize appearance of th…
…is button. (#11462) (#11605)

* 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.

Dreadnoth added a commit to Dreadnoth/kibana that referenced this pull request Aug 8, 2017

Create CollapseButton component class to standardize appearance of th…
…is button. (elastic#11462) (elastic#11605)

* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment