-
Notifications
You must be signed in to change notification settings - Fork 157
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
Add optimise timeline feature #2180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just one nitpick:
The "state" of the button doesn't persist across refreshes. The "breakdown by file type" and "merge all groups" checkboxes retain their state when refreshed, so I think it would be good for the new checkbox to follow this behaviour as well!
@sopa301 thank you and fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start @jonasongg! The optimised timelines are looking good and would definitely improve the usability of RepoSense!
While most of the changes you've made look good, there are some things left out and some design decisions to discuss too.
- [High priority] The chart zoom feature (selected using Ctrl/Cmd + click) does not work correctly when the timelines are optimised, i.e., the dates used for the zooming in are the same as the original dates instead of the optimised dates, which is misleading to the user.
- [Low priority] When I open the Commits Panel for an optimised chart, the chart shown at the top of the Commits Panel is not optimised.
- [Discuss] When the report is grouped by repos, the various authors under the repo may have different timelines. This discrepancy may make it difficult to compare when different teammates are contributing relatively. We may want to make the timeline duration consistent across a repo group (@damithc). However, when grouping by authors, this doesn't matter.
- [Discuss] We may want to think of a more intuitive name for "Optimise timeline", because it is not immediately clear what this feature does to end users. I would suggest something along the lines of "Condense charts".
Good inputs @ckcherry23
One option (probably easier too) is to take the stance that 'optimized' timelines isn't suitable for timeline comparisons. What do you all think?
'Trim'? |
@ckcherry23 @damithc thank you for your comments! will do the necessary changes asap. as for when we are grouping by repos, it shouldn't be too difficult to make the timeline consistent across a group (i think we can just select the minimum and maximum commit date of the group instead of the user). i'm ok to implement either option! i think "trim timeline" works! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @jonasongg! The ramps and the chart zoom work well now with the trim timelines feature!
However, fixing the zoom feature broke the optimization of the default ramp chart shown on the Commits Panel when "trim timelines" is enabled, sorry for the oversight. Maybe the best option is to pass the optimized timeline min/max dates conditionally along with filterSinceDate
and filterUntilDate
for this in lines 103
and 221
of c-summary-charts.vue
?
The rest of the changes look good to me. As discussed, since we need other collaborators' help for adding Cypress tests, I think we can ignore that comment and ask for our second reviewer feedback now.
…line' into add-optimise-timeline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for this @jonasongg! This would be a super useful feature for setting up personal RepoSense reports!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested and works great. Definitely a very useful feature to have.
Regarding adding of other tests, such as to test it with the zoom feature in chartView_zoomFeature.cy.js
, let's create an issue for these so we don't forget about it.
@MarcusTXK the tests were actually added in this PR under optimiseTimeline.cy.js - should I change this? |
@jonasongg Ah OK, apologies I missed it. No worries, this works, let's leave it as it is then. |
The following links are for previewing this pull request:
|
Fixes #2162
Part of #2157
Proposed commit message
Other information