Skip to content
This repository was archived by the owner on Apr 11, 2019. It is now read-only.

Fixes #191 : Solves filtering changesets bug #193

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gabru-md
Copy link

@gabru-md gabru-md commented Jun 14, 2018

Fixes #191

The bug is that when we filter the data and click on the changesets,
then instead of taking to another page as it should do, the filter gets
cleared and all changesets are visible. By removing onBlur from the
attributes this problem is easily solved.

Cheers!


This change is Reviewable

@jankeromnes
Copy link
Contributor

jankeromnes commented Jul 12, 2018

Hi @gabru-md, thanks for fixing this problem!

I confirm that your commit does fix it, but I find it a little confusing that you can have an "active filter" while the form gets automatically hidden (you might forget about it, and then wonder why so few changesets are being shown).

How about we don't hide the form when it contains a filter value?

You can achieve this by making the "auto-hide" mechanism conditional on the form being empty, e.g.:

  • Add a conditional "empty" or "auto-hide" class to the <form> element in src/components/changesetFilter.jsx with an attribute like className={!value ? 'empty' : ''}
  • Fix the <form> auto-hiding style in src/styles.css by replacing form:not(:focus-within) { opacity: 0; } with form.empty:not(:focus-within) { opacity: 0; } (basically just add .empty to the selector)
  • Fix the <button> auto-hiding style by replacing form:focus-within + button { display: none; } with form:not(.empty) + button, form:focus-within + button { display: none; } (basically add a second selector)

@armenzg
Copy link
Contributor

armenzg commented Jul 17, 2018

Hi @gabru-md
Please let us know if you're still working on this or not.

Take care,
Armen

@gabru-md
Copy link
Author

I am sorry for not being able to update this PR for this long. 😅
I will make the changes and ping you once done.
😸

@jankeromnes
Copy link
Contributor

That's perfect, thanks a lot! Also no need to apologise (I'm the one who took 3 weeks to reply 😋)

Cancel button clears out the filter and calls the onChange function to trigger the event.

Cheers!
@gabru-md
Copy link
Author

@armenzg @jankeromnes I've done some changes you can check here. I've added the cancel button that triggers an event causing the changesets to reset to original state.

@jankeromnes
Copy link
Contributor

jankeromnes commented Jul 23, 2018

Thanks for implementing a cancel button! I have a few minor nits about the UX:

cancel-coverage-filter-button

  • On Ubuntu, the button and input heights are different in Chrome (but they do have the same height in Firefox). You could work around this by transforming the full button into just a clickable cancel icon (e.g. by hiding all the browser button styles), and overlaying it on top of the input (e.g. inside right of the input)
  • When you click the button, it does clear the filter, but the empty input field remains visible (because the button still has focus inside the form). It would be nice to automatically .blur() the cancel button when it was clicked.
  • I'm not sure this suggestion is compatible with my other suggestions, but have you tried using an ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants