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

[#2130] Add highlight and scroll to group #2131

Merged
merged 24 commits into from
Apr 18, 2024

Conversation

jonasongg
Copy link
Contributor

@jonasongg jonasongg commented Feb 19, 2024

Fixes #2130

Proposed commit message

Add highlight and scroll to group

It can be useful to have a way to send a RepoSense link that
automatically scrolls to and highlights the group that the sender
thinks the recipient should pay attention to, saving them time to find
the repo in question.

Let's add this functionality to add convenience when sending around
RepoSense reports.

Other information

No need for this anymore now that I have changed the logic to piggyback off activeRepo

Interactions with other features (do suggest any I've missed/any desired behaviour!):
Merging groups: Highlight doesn't show on a merged group, but is retained if group is expanded. Refreshing navigates back to the highlighted repo's merged group
Changing group by: Highlight remains on the same "relative repo" (for example, second from the top). Possibly undesirable, will change it to remove the highlight on group-by change
Sorting
Filtering/searching

@jonasongg jonasongg requested a review from a team February 19, 2024 18:25
},

highlightRepo(groupIndex: number, index: number): void {
const offset = this.filteredRepos
Copy link
Contributor Author

@jonasongg jonasongg Feb 19, 2024

Choose a reason for hiding this comment

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

This part is a bit strange (the need to calculate an offset) because of the way the refs are stored for summary-chart. Basically, there are multiple groups and in each group, multiple repos. But the number X in the ref for summary-chart-X refers to the index of the repo in the individual group. This means that in each ref, there is actually an array of div elements from multiple groups. I didn't want to change the way the refs were stored since the iframe feature seems to depend on it

@jonasongg jonasongg marked this pull request as draft February 19, 2024 18:34
@sopa301
Copy link
Contributor

sopa301 commented Feb 20, 2024

This looks pretty neat! I'm thinking that for "changing group by", if we highlight a repo under a user, then switching the "group by" can highlight that user under the repo? This would probably require some searching and storing of repo and user information, but it would keep the consistency between the grouping orders.

@damithc
Copy link
Collaborator

damithc commented Feb 20, 2024

@jonasongg thanks for working on this. Also note that clicking on the </> icon already highlights a specific repo/user. Probably we can piggyback on that existing functionality (i.e., add the auto-scroll to the generated URL)?

@jonasongg jonasongg marked this pull request as ready for review March 11, 2024 06:42
@jonasongg
Copy link
Contributor Author

@damithc thanks prof for this! with further discussion with @ckcherry23, i decided it was best to not change/add too many unnecessary and confusing features. to keep it simple, i made it such that the active repo (pressing the </> or list icon) will be scrolled to automatically, even on refresh. let me know what you think!

@damithc
Copy link
Collaborator

damithc commented Mar 11, 2024

@damithc thanks prof for this! with further discussion with @ckcherry23, i decided it was best to not change/add too many unnecessary and confusing features. to keep it simple, i made it such that the active repo (pressing the </> or list icon) will be scrolled to automatically, even on refresh. let me know what you think!

@jonasongg I agree, best not to add too many similar features. What's the original behavior and how does this PR changes it?

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.

Thanks for implementing this @jonasongg! We might have to make some changes so that the original behaviour is not affected much (more given below).

frontend/src/components/c-summary-charts.vue Show resolved Hide resolved
frontend/src/components/c-summary-charts.vue Outdated Show resolved Hide resolved
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.

Thanks for the quick update @jonasongg. Can we also add some Cypress tests to make sure this functionality works?
Also, please remember to answer prof Damith's query regarding how the current behaviour has been modified.

@jonasongg
Copy link
Contributor Author

@damithc the original behaviour was that pressing either the list or code icon would highlight the repo and the commit/code panel would appear on the right. this PR makes it such that highlighting the repo would also cause it to be in focus if it's not within view (thanks @ckcherry23 for this suggestion!). this would mostly happen if the user reloads the page or reopens the link somehow (so the page will automatically scroll to the highlighted repo).

@damithc
Copy link
Collaborator

damithc commented Mar 14, 2024

@damithc the original behaviour was that pressing either the list or code icon would highlight the repo and the commit/code panel would appear on the right. this PR makes it such that highlighting the repo would also cause it to be in focus if it's not within view (thanks @ckcherry23 for this suggestion!). this would mostly happen if the user reloads the page or reopens the link somehow (so the page will automatically scroll to the highlighted repo).

@jonasongg Got it. Sounds good.

@jonasongg
Copy link
Contributor Author

@ckcherry23 i added a cypress test, but for some reason the test seems flaky - the scroll in the cypress window seems to randomly stop at the div before the highlighted one and sometimes it works fine. manually testing this behaviour outside of cypress, this issue seems to never occur. for now, i added some retries to the flaky test. if possible, could you help me see if this flaky behaviour can be replicated on your end? meanwhile i will continue investigating!

@ckcherry23
Copy link
Member

@ckcherry23 i added a cypress test, but for some reason the test seems flaky - the scroll in the cypress window seems to randomly stop at the div before the highlighted one and sometimes it works fine. manually testing this behaviour outside of cypress, this issue seems to never occur. for now, i added some retries to the flaky test. if possible, could you help me see if this flaky behaviour can be replicated on your end? meanwhile i will continue investigating!

Hi @jonasongg, I've investigated the issue you're facing, and seems like it's a bug in our testing framework Cypress. Using scroll-behavior: smooth in Cypress tests broke actionability like click() and check() in an older version. Although there seems to have been a fix for that issue, there is probably an edge case occurring in our code that is causing the same issue. You can remove the scroll-behavior CSS and verify that your test case passes without retries, to ensure that this is the root cause of the error.

As a workaround, you can disable smooth scrolling for that particular element before starting the test, and enable it after the test is complete. You can refer to some solutions for this mentioned in the same issue.

It would be great if we didn't have to use retries for this test, as we would expect such a feature to work reliably. If there is no possible workaround for this, then perhaps we can disable the smooth-scrolling animation and opt for an instant scroll instead.

@jonasongg
Copy link
Contributor Author

@ckcherry23 thank you for helping investigate this!! i think the least complicated way to fix this issue is to just remove the behavior: 'smooth' from scrollIntoView({ behavior: 'smooth', block: 'nearest' }) - the scrollBehaviour is not attached to any dom element, so i imagine it would be quite complicated to disable smooth scrolling just for cypress. since the smooth scrolling is just an cosmetic effect, i don't think we will lose too much just removing it?

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!

@ckcherry23 ckcherry23 requested a review from a team April 1, 2024 17:28
Copy link
Member

@MarcusTXK MarcusTXK left a comment

Choose a reason for hiding this comment

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

LGTM, great job!

@ckcherry23 ckcherry23 merged commit f07af5a into reposense:master Apr 18, 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.

Add a way to highlight and scroll a particular repo/author
5 participants