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

[#581] CodeView#filter: add interactive selection switch #608

Merged
merged 13 commits into from Mar 28, 2019
Merged

[#581] CodeView#filter: add interactive selection switch #608

merged 13 commits into from Mar 28, 2019

Conversation

fzdy1914
Copy link
Member

@fzdy1914 fzdy1914 commented Mar 19, 2019

Fixes #581

The filter selection's radio button serves nothing more than an "indicator".
Changing option doesn't do anything until the corresponding controls are
modified.

It is redundant and inconvenient for users to manually switch the selection.

Instead of disabling the deselected option, let's allow interaction switch
the selection.

@fzdy1914 fzdy1914 requested a review from a team March 19, 2019 03:42
Copy link
Contributor

@chel-seyy chel-seyy left a comment

Choose a reason for hiding this comment

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

Perhaps the indicated selection (i.e. radio button) can update when the user presses but have not entered anything into the filter box. This can provide more user feedback by showing that the selection has changed.

@fzdy1914
Copy link
Member Author

Perhaps the indicated selection (i.e. radio button) can update when the user presses but have not entered anything into the filter box. This can provide more user feedback by showing that the selection has changed.

Have edited it as request.

# Conflicts:
#	frontend/src/index.jade
#	frontend/src/static/js/v_authorship.js
@fzdy1914 fzdy1914 requested a review from a team March 21, 2019 05:25
@yamidark yamidark requested a review from chel-seyy March 21, 2019 12:04
Copy link
Contributor

@chel-seyy chel-seyy left a comment

Choose a reason for hiding this comment

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

I notice the filter box is not "disabled" (looks grey) after switching. Is it intended this way?

@fzdy1914
Copy link
Member Author

I notice the filter box is not "disabled" (looks grey) after switching. Is it intended this way?

It should be the intended behavior mentioned in #581 .

# Conflicts:
#	frontend/src/index.jade
@damithc
Copy link
Collaborator

damithc commented Mar 23, 2019

hmm... after playing around with the feature, it feels like the previous state was more intuitive although it takes an extra click. What do you guys think?
Perhaps we should allow both ways of switching between the two modes?

@fzdy1914
Copy link
Member Author

hmm... after playing around with the feature, it feels like the previous state was more intuitive although it takes an extra click. What do you guys think?
Perhaps we should allow both ways of switching between the two modes?

@eugenepeh @yamidark @yong24s Your opinions on this?

@eugenepeh
Copy link
Member

@eugenepeh @yamidark @yong24s Your opinions on this?

Sounds good to me

@yamidark
Copy link
Contributor

@eugenepeh @yamidark @yong24s Your opinions on this?

Sounds good to me as well

@fzdy1914
Copy link
Member Author

@damithc I have changed as request, please review the latest commit.

Copy link
Collaborator

@damithc damithc left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Copy link
Contributor

@ongspxm ongspxm left a comment

Choose a reason for hiding this comment

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

minor nits

frontend/src/index.jade Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #608      +/-   ##
============================================
- Coverage     80.64%   80.22%   -0.42%     
+ Complexity      537      522      -15     
============================================
  Files            68       68              
  Lines          1767     1745      -22     
  Branches        185      179       -6     
============================================
- Hits           1425     1400      -25     
- Misses          247      251       +4     
+ Partials         95       94       -1
Impacted Files Coverage Δ Complexity Δ
...ain/java/reposense/parser/RepoConfigCsvParser.java 84.21% <0%> (-5.45%) 5% <0%> (-3%)
...c/main/java/reposense/model/RepoConfiguration.java 81.81% <0%> (-3.9%) 58% <0%> (-8%)
src/main/java/reposense/parser/CsvParser.java 67.56% <0%> (-0.73%) 15% <0%> (-4%)
...main/java/reposense/model/AuthorConfiguration.java 85.05% <0%> (+0.53%) 29% <0%> (ø) ⬇️

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 2c25cfd...b08199c. Read the comment docs.

@eugenepeh eugenepeh merged commit 0d04311 into reposense:master Mar 28, 2019
@fzdy1914 fzdy1914 deleted the indicator branch March 28, 2019 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants