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.

Sign up for GitHub

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

Jump to bottom

fix toggle any filter #1065

Merged
merged 2 commits into from
May 4, 2018
Merged

fix toggle any filter #1065

merged 2 commits into from
May 4, 2018

Conversation

joshmorel
Copy link
Contributor

@joshmorel joshmorel commented May 3, 2018 •
edited
Loading

@thelostone-mc - please take a look, I found passing the event to the now renamed toggleAny function made it easier to reason about and achieve the expected behaviour.

Description

Fix toggle any filter - could not previously click "Any"

Checklist
  • [x ] linter status: 100% pass
  • [ x] changes don't break existing behavior
  • [ x] commit message follows commit guidelines
    for multiselect checkbox groups:
    if all selected --> others deselected
    if other selected --> any deselected
    if none selected --> any selected
Affected core subsystem(s)

UI

Testing

Confirmed XHR request matches to existing app for:

  1. Initial load
  2. Select another option
  3. Deselect all options within group (any)
  4. Select other option from any
  5. Select from techstack

Note that I noticed a weird issue with local storage that is also in production for which an issue was created.

Refers/Fixes

Fixes: #1011

@joshmorel
fix toggle any filter
fef2ade 
for multiselect checkbox groups:
if all selected --> others deselected
if other selected --> any deselected
if none selected --> any selected
stickler-ci
stickler-ci reviewed May 3, 2018
View reviewed changes
Copy link

@stickler-ci stickler-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your eslint configuration output the following error:

ESLint couldn't find the plugin "eslint-plugin-html". This can happen for a couple different reasons:

The above plugin is not installed.

stickler-ci
stickler-ci reviewed May 3, 2018
View reviewed changes
Copy link

@stickler-ci stickler-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your eslint configuration output the following error:

ESLint couldn't find the plugin "eslint-plugin-html". This can happen for a couple different reasons:

The above plugin is not installed.

@codecov
Copy link

codecov bot commented May 3, 2018 •
edited
Loading

Codecov Report

Merging #1065 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1065      +/-   ##
==========================================
- Coverage   32.02%   31.98%   -0.05%     
==========================================
  Files         105      105              
  Lines        7247     7247              
  Branches      941      941              
==========================================
- Hits         2321     2318       -3     
- Misses       4818     4821       +3     
  Partials      108      108
Impacted Files Coverage Δ
app/app/settings.py 80.79% <0%> (-1.99%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f45d40f...8438d45. Read the comment docs.

thelostone-mc
thelostone-mc approved these changes May 4, 2018
View reviewed changes
Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x

LGTM ^_^

Thanks @joshmorel

cc: @mbeacom -> this is good for merge from my side

app/assets/v2/js/pages/dashboard.js
}
if (event.target.value === 'any') {
// Deselect other filters when 'any' is selected
$('input[name=' + key + '][value!=any]').prop('checked', false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

mbeacom
mbeacom approved these changes May 4, 2018
View reviewed changes
Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - Thanks for the contribution!

@mbeacom mbeacom merged commit fbf992a into gitcoinco:master May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stickler-ci stickler-ci stickler-ci left review comments

@mbeacom mbeacom mbeacom approved these changes

@thelostone-mc thelostone-mc thelostone-mc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joshmorel @thelostone-mc @mbeacom @stickler-ci