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

[#2148] Show tags on the ramp chart #2163

Merged
merged 30 commits into from
Apr 27, 2024

Conversation

jonasongg
Copy link
Contributor

@jonasongg jonasongg commented Mar 18, 2024

Fixes #2148

Proposed commit message

Show tags of repo on the ramp chart

It can be useful to view all the tags of each repo that appears on the
ramp chart. Though this information is visible if the repo is merged
and the user click to see the breakdown of each commit, it can still be
useful to see this information upfront (for searching Ctrl+F perhaps).
This feature would be toggleable with a checkbox to reduce clutter.

Let's add this functionality to add convenience when searching through
RepoSense reports with tags.

Other information

Pending changes to documentation and tests!

@jonasongg jonasongg requested a review from a team March 19, 2024 07:15
@sopa301
Copy link
Contributor

sopa301 commented Mar 20, 2024

Would it be useful to have the tags direct to the tag page (e.g. https://github.com/reposense/RepoSense/releases/tag/v2.5)? The URLs can be reconstructed from the tag names and the repo URL.

@damithc
Copy link
Collaborator

damithc commented Mar 20, 2024

@jonasongg can you post a screenshot of how the feature appears. I couldn't see any tags in the dashboard preview (I guess the repo we use for the preview doesn't have any tags?).

@jonasongg
Copy link
Contributor Author

@damithc
here's a screenshot showing:

  1. the checkbox to toggle viewing all tags on top
  2. what it looks like if the tags overflow into a new line
  3. what it looks like if the tags don't overflow
    image

@jonasongg
Copy link
Contributor Author

@sopa301 that's a great suggestion, thanks!

@damithc
Copy link
Collaborator

damithc commented Mar 22, 2024

Thanks for the screenshot @jonasongg
Looks good. Perhaps view repo tags -> show tags?

@jonasongg
Copy link
Contributor Author

@damithc changed the name, thanks!

Copy link
Contributor

@sopa301 sopa301 left a comment

Choose a reason for hiding this comment

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

LGTM!

@sopa301 sopa301 requested a review from a team March 29, 2024 11:13
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @jonasongg!

Can I check what the behaviour is like if we sort by author, and there's multiple different repositories under the same author? I haven't tested myself, but it seems that the current version will display all the tags (which may be from different repositories) at the top (author) level

@jonasongg
Copy link
Contributor Author

@vvidday thanks for spotting this! i changed it such that if we group by author, the tags will appear on the repo (second) level instead. i also made it such that viewing all tags is stored in the url

@ckcherry23 ckcherry23 requested a review from vvidday April 13, 2024 10:01
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @jonasongg, definitely agree with the new proposed way of displaying tags when group by author is selected. And great job on adding the state of the toggle to the url!

Have a question about the logic of getting tag links when grouped by author:

frontend/src/components/c-summary-charts.vue Outdated Show resolved Hide resolved
frontend/src/components/c-summary-charts.vue Outdated Show resolved Hide resolved
frontend/src/components/c-summary-charts.vue Outdated Show resolved Hide resolved
@ckcherry23 ckcherry23 requested a review from vvidday April 15, 2024 05:17
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

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

Sorry for the delay - LGTM

@vvidday vvidday requested a review from a team April 23, 2024 13:20
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

Great work @jonasongg! I think we can go ahead once we have a test for the group by author case too.

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a Cypress test for displaying tags when grouped by author too?

Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

LGTM @jonasongg!
As discussed, let us create an issue for the remaining Cypress test required.

(cherry picked from commit 3b65d51)
(cherry picked from commit 5c8411c)
@sopa301 sopa301 merged commit 02c7198 into reposense:master Apr 27, 2024
8 checks passed
Copy link
Contributor

The following links are for previewing this pull request:

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.

Show tags on the ramp chart
5 participants