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

RampChart: add group by authors option #570

Merged
merged 37 commits into from Mar 21, 2019

Conversation

chel-seyy
Copy link
Contributor

@chel-seyy chel-seyy commented Feb 24, 2019

Fixes #543.

Users are only allowed to group the ramp charts by repository.

While grouping by repository allows users to have a rough comparison of
contributions between authors of the same repository, it does not
provide any insight into an author's contribution data across all
repositories he has contributed to.

Let's add the grouping of authors as an option for users to select, and
this can work towards the goal of having a toolbar[1] at the top of the
report.

[1]: Sorting options graphical goal:
https://github.com/orgs/reposense/teams/devs/discussions/1

frontend/src/static/js/v_summary.js Outdated Show resolved Hide resolved
frontend/src/static/js/v_summary.js Outdated Show resolved Hide resolved
frontend/src/static/js/v_summary.js Outdated Show resolved Hide resolved
@chel-seyy chel-seyy requested a review from a team February 27, 2019 02:21
Copy link
Member

@fzdy1914 fzdy1914 left a comment

Choose a reason for hiding this comment

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

From my part, the way to choose which to group is a little bit weird.
image
Can consider to make it a drop-down choice just like sort by option. It is just a suggestion anyway. Maybe you can ask prof for his option.

@chel-seyy
Copy link
Contributor Author

Hmm I see, but sorting of the ramp charts is done in the select-menu, where the user can sort by author name, contribution etc. So changing this to sort by may be confusing.
@damithc your opinion on this?

@damithc
Copy link
Collaborator

damithc commented Feb 27, 2019

Hmm I see, but sorting of the ramp charts is done in the select-menu, where the user can sort by author name, contribution etc. So changing this to sort by may be confusing.
@damithc your opinion on this?

I think he meant that we can use a drop-down instead of radio buttons, to follow the style of sort by.

@damithc
Copy link
Collaborator

damithc commented Feb 27, 2019

At one point we mentioned about aiming for something like this. So, it does make sense to make it a drop-down.
image

@fzdy1914
Copy link
Member

I think he meant that we can use a drop-down instead of radio buttons, to follow the style of sort by.

@chelseyong Sorry for not explaining clearly. Using a drop-down is what I mean.

@chel-seyy chel-seyy requested a review from a team February 28, 2019 07:26
Copy link
Member

@fzdy1914 fzdy1914 left a comment

Choose a reason for hiding this comment

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

Maybe it is better to put group by section before sort by section?
So the order makes more sense.

@chel-seyy chel-seyy requested a review from a team March 4, 2019 11:07
Copy link
Member

@eugenepeh eugenepeh left a comment

Choose a reason for hiding this comment

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

Another interesting bug, notice how the display name has changed according to the reverse checkbox.
image
image

@chel-seyy
Copy link
Contributor Author

Hmm.. interesting indeed.
For these 2 repositories, despite both having the same username, they have different display name.
When the order is reversed, since the code takes the first repository's display name, the author's name will now be different.

However, the situation that 2 similar users have different display name but same username is quite rare.
One possible alternative is to display the username beside the author (in brackets) to reduce user's confusion. Eg: Eugene (eugenepeh)

@chel-seyy
Copy link
Contributor Author

chel-seyy commented Mar 20, 2019

codacy kept failing.. The logic for updateSortSelection seems simple though.

@yamidark
Copy link
Contributor

codacy kept failing.. The logic for updateSortSelection seems simple though.

If you are unable to resolve this Codacy issue, we can just ignore it for now, not much of an issue.

@eugenepeh
Copy link
Member

However, the situation that 2 similar users have different display name but same username is quite rare.
One possible alternative is to display the username beside the author (in brackets) to reduce user's confusion. Eg: Eugene (eugenepeh)

Yes, that's a good way to resolve this problem.

@chel-seyy
Copy link
Contributor Author

@eugenepeh thanks! Can this be merged soon to proceed with #601?

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.

Nice 👍

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 Show resolved Hide resolved
sortFiltered() {
let full = [];
if (this.filterGroupSelection === 'groupByNone') {
full[0] = this.groupByNone(this.filtered);
Copy link
Contributor

Choose a reason for hiding this comment

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

comment that this is making everything "under one group", not apparent on first glance

@yamidark
Copy link
Contributor

yamidark commented Mar 21, 2019

Nits to proposed message:

Currently, Users are only allowed to group the ramp charts by repository.

While grouping by repository allows users to have a rough comparison of contributions
between authors of the same repository, it does not provide any

They are not able to group them by authors, which can provide greater insight
into an author's contribution data, across all repositories he has contributed to.

Let's add the grouping of authors as an option for users to select, and this can
work towards the goal of having a toolbar[1] at the top of the dashboardreport.

[1]: Sorting toolbar for referenceSorting options graphical goal:
https://github.com/orgs/reposense/teams/devs/discussions/1

@yamidark yamidark changed the title Allow grouping of ramp chart via authors RampChart: add grouping via authors option Mar 21, 2019
@yamidark yamidark changed the title RampChart: add grouping via authors option RampChart: add group via authors option Mar 21, 2019
@yamidark yamidark changed the title RampChart: add group via authors option RampChart: add group by authors option Mar 21, 2019
@yamidark yamidark merged commit bde1650 into reposense:master Mar 21, 2019
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