Skip to content
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

Add ability to show/hide filter bar #17161

Merged
merged 1 commit into from Apr 30, 2018

Conversation

Projects
None yet
8 participants
@aaronoah
Copy link
Contributor

aaronoah commented Mar 14, 2018

This PR added a feature to show/hide filter bar for the filter_bar component using a similar button in the sidebar.

Closes #11752.

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Mar 14, 2018

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@lukasolson lukasolson changed the title Closes #11752 Add ability to show/hide filter bar Mar 15, 2018

@lukasolson lukasolson requested review from lukasolson and Bargs Mar 15, 2018

@karmi

This comment has been minimized.

Copy link
Member

karmi commented Mar 20, 2018

Hi @aaronoah, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@aaronoah

This comment has been minimized.

Copy link
Contributor Author

aaronoah commented Mar 20, 2018

Sorry. I've included my email to it.

@aaronoah

This comment has been minimized.

Copy link
Contributor Author

aaronoah commented Mar 20, 2018

So sorry guys for your review, I try to fix my commits but there were many new commits by the time I rebase. [Now it is back to easier mode]

@aaronoah aaronoah force-pushed the aaronoah:master branch from 5ed8c5f to 8c78d9e Mar 20, 2018

@aaronoah

This comment has been minimized.

Copy link
Contributor Author

aaronoah commented Mar 25, 2018

Hi guys! I'm open to any comments on this PR, feel free to give me some feedback and I'll try to improve it.

@Bargs

This comment has been minimized.

Copy link
Contributor

Bargs commented Mar 26, 2018

Hey @aaronoah, sorry for the silence, we'll take a look at this as soon as we can, we're just spread a little thin at the moment.

@Bargs Bargs requested review from thomasneirynck and removed request for lukasolson and Bargs Mar 28, 2018

@thomasneirynck

This comment has been minimized.

Copy link
Contributor

thomasneirynck commented Apr 5, 2018

hi @aaronoah , thanks a lot of this improvement,

I'm going to try and take at look at this if not today, then tomorrow, apologies for the delay on this one

@thomasneirynck

This comment has been minimized.

Copy link
Contributor

thomasneirynck commented Apr 5, 2018

jenkins, test this

@thomasneirynck

This comment has been minimized.

Copy link
Contributor

thomasneirynck commented Apr 5, 2018

hi @aaronoah,

I may be missing something super obvious here, but where is the button?

image

the ng-hide class seems always applied, filters or no filters.

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Apr 5, 2018

@aaronoah

This comment has been minimized.

Copy link
Contributor Author

aaronoah commented Apr 5, 2018

The feature is that when the last number of filter pill extend to the next row, the icon will appear. Only when there are multiple rows will it appear.

@thomasneirynck
Copy link
Contributor

thomasneirynck left a comment

Functionality works well imo, thanks!

Hoping we could avoid importing a copy-paste of an existing icon.

ng-class="filterNavToggle.isOpen == true ? '' : 'filter-nav-link--close'"
aria-hidden="!filters.length || !showCollapseLink()"
>
<a class="filter-nav-link__anchor" ng-click="toggleFilterShown($event)">

This comment has been minimized.

Copy link
@thomasneirynck

thomasneirynck Apr 5, 2018

Contributor

Can we just use the existing circle-down font-awesome icon, similar to how we use the circle-left icon for the sidebar? https://fontawesome.com/icons/chevron-circle-down?style=solid

cf.

const $icon = $('<span class="kuiIcon fa-chevron-circle-left"></span>');

This comment has been minimized.

Copy link
@aaronoah

aaronoah Apr 5, 2018

Author Contributor

Originally I thought I should use a similar icon on the app-switcher sidebar. It makes sense to avoid duplicates. I will switch to font-awesome.

@thomasneirynck thomasneirynck requested a review from cchaos Apr 5, 2018

@cchaos
Copy link
Contributor

cchaos left a comment

I was able to take a quick look look. The functionality does work well and having that logic already is awesome. I agree with @thomasneirynck that we shouldn't be duplicating icons. I also noticed that there isn't enough space provided for the icon at certain screen widths:

screen shot 2018-04-05 at 17 22 19 pm

My suggestion would actually be to put the arrow on the left side of the filter bar and add padding to the filter bar only when the icon shows up. This will also fix the icon from jumping around the UI from collapsed to uncollapsed view.

@aaronoah

This comment has been minimized.

Copy link
Contributor Author

aaronoah commented Apr 5, 2018

@cchaos It's a good suggestion when I try to do this feature. Though putting the icon on the left avoids jumping of the icon, I realize that it might be weird to add padding to the icon when filter pills grow to multiple rows and will trigger re-layout of all the pills.

@cchaos
Copy link
Contributor

cchaos left a comment

@aaronoah I see the issue there. Then what I would do is move it to the far side of the filter bar and have the Actions button stay on the left since it shifts to the bottom of the bar anyway. This will keep the collapsing button in a permanent spot so users don't have to move their mouse to re-collapse.

If outlined how I would do it below.


.filter-panel__pill {
display: inline-block;
width: 90%;

This comment has been minimized.

Copy link
@cchaos

cchaos Apr 6, 2018

Contributor

Change to width: calc(100% - 80px). At 90%, there is still the chance of overlap.

@@ -76,6 +95,34 @@ filter-bar {
line-height: 1.5;
}

.action-show {
position: absolute;
right: 10px;

This comment has been minimized.

Copy link
@cchaos

cchaos Apr 6, 2018

Contributor

Position it left of the nav-link__icon: right: 30px

display: inline;
position: absolute;
top: 10px;
right: 15px;

This comment has been minimized.

Copy link
@cchaos

cchaos Apr 6, 2018

Contributor

Keep tight to the right hand side padding which is 10px, so change to right: 10px

position: absolute;
top: 10px;
right: 15px;
opacity: 0.35;

This comment has been minimized.

Copy link
@cchaos

cchaos Apr 6, 2018

Contributor

Unfortunately/fortunately we have to be AA contrast compliant for accessibility, the lightest we can go here would be opacity: 0.75

}

.filter-nav-link--close {
right: 75px;

This comment has been minimized.

Copy link
@cchaos

cchaos Apr 6, 2018

Contributor

No longer need this to accommodate the action button.

@thomasneirynck thomasneirynck self-requested a review Apr 6, 2018

@aaronoah

This comment has been minimized.

Copy link
Contributor Author

aaronoah commented Apr 6, 2018

@cchaos Thanks for your suggestions! It makes sense to move it to the far right. I thought I should not change the position of actions button but if it's not an issue then I would try to do it as you said. @thomasneirynck

@thomasneirynck

This comment has been minimized.

Copy link
Contributor

thomasneirynck commented Apr 6, 2018

yup in agreement with @cchaos , thx @aaronoah!

@thomasneirynck
Copy link
Contributor

thomasneirynck left a comment

thanks for the modifications @aaronoah!

I'd consider a few things:

  1. when the bar is collapsed, making it really small can have the filter overlap with the text

e.g.:
image

This does not occur when the bar is expanded, then the text flows under it naturally
e.g.:
image

I think we probably should avoid the overlap.

  1. when the filter-bar is collapsed, it isn't very clear to the user that there are actually missing filters. imho it would make sense to add some text/feedback that filters are missing. Perhaps next to the expand-icon, something like ... expand to see all filters (v). Feel free to make other suggestion here, I'm not 100% sure what the best approach would be.
@aaronoah

This comment has been minimized.

Copy link
Contributor Author

aaronoah commented Apr 11, 2018

  1. I'd consider that it might be rare for users to make the width of a window too small, which also makes some part of search bar invisible. So I propose to add min-width to the filter bar so that it won't resize under the limit.

4

  1. I propose to replace tooltip texts "Expand filter bar" with "Expand to show filters" and "Collapse filter bar" with "Collapse to hide filters" for conciseness.
    @thomasneirynck

If something does not work out, let me know and I can try to search for other approaches.

@thomasneirynck

This comment has been minimized.

Copy link
Contributor

thomasneirynck commented Apr 25, 2018

@aaronoah, agreed on both counts.

  1. Adding min-width . Yup, I'd put it in. @cchaos, do you agree?
  2. OK with renaming of texts.
@cchaos

This comment has been minimized.

Copy link
Contributor

cchaos commented Apr 25, 2018

I think this is fine for now. We will be updating the global time picker and filter bar to an EUI component in the near future. elastic/eui#668

@cchaos

cchaos approved these changes Apr 25, 2018

@thomasneirynck

This comment has been minimized.

Copy link
Contributor

thomasneirynck commented Apr 27, 2018

hi @aaronoah

it seems we're good to go then for (1). Maybe we can still do the rename of (2) for clarity.

thx @cchaos!

@thomasneirynck
Copy link
Contributor

thomasneirynck left a comment

lgtm pending green CI

@thomasneirynck

This comment has been minimized.

Copy link
Contributor

thomasneirynck commented Apr 27, 2018

jenkins, test this

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Apr 27, 2018

@thomasneirynck

This comment has been minimized.

Copy link
Contributor

thomasneirynck commented Apr 28, 2018

jenkins, test this

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Apr 28, 2018

@aaronoah

This comment has been minimized.

Copy link
Contributor Author

aaronoah commented Apr 28, 2018

In the output, I saw some weird issues from moment.js and some unhandled Promise rejections that do not relate to this PR. I will try to find some ways to resolve it.

I think there are too many commits after this PR in the master branch, so if I reset my commits and rebase on the latest master and apply this PR again, it might solve the problem.

@aaronoah aaronoah force-pushed the aaronoah:master branch 2 times, most recently from 82d5156 to 0f25e64 Apr 30, 2018

@aaronoah aaronoah force-pushed the aaronoah:master branch from 0f25e64 to 05477cf Apr 30, 2018

@thomasneirynck

This comment has been minimized.

Copy link
Contributor

thomasneirynck commented Apr 30, 2018

jenkins, test this

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Apr 30, 2018

@thomasneirynck thomasneirynck merged commit 81862d1 into elastic:master Apr 30, 2018

2 checks passed

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

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Apr 30, 2018

thomasneirynck added a commit that referenced this pull request Apr 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.