Remove stickyMonitor from diff_file_head.vue
What does this MR do?
Related to: #35814 (closed)
Remove stickyMonitor
from diff_file_head.vue
In this scenario, stickyMonitor only adds the class is-stuck
to diff_file_header. On large diffs, this almost doubles the render time.
All the is-stuck
class does is set border-radius to 0. I believe the performance impact is not worth the trade-off.
Screenshots
The visual difference in changes tabs.
Load difference ~40% reduction in time
Testing
If testing with local seeded projects use this one: http://localhost:3000/Commit451/lab-coat/merge_requests/2/diffs
That is the one I used for my testing.
Here are the stats for that project at the time of writing this MR
Does this MR meet the acceptance criteria?
Conformity
Merge request reports
Activity
changed milestone to %12.6
added backstage [DEPRECATED] frontend groupsource code labels
assigned to @jboyson
assigned to @iamphill
@iamphill tech review please
@annabeldunstone ux review please
Please note, the borders will stay rounded with this change. The code required to "unround" them increases the load on my test MR from 4.8 to 8.2 seconds
I do not believe the performance hit is worth it for such a small visual change.cc: @andr3
assigned to @annabeldunstone
Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.
Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.
Category Reviewer Maintainer frontend Daniel Cipoletti ( @dcipoletti
)Tim Zallmann ( @timzallmann
)Generated by
DangerEdited by 🤖 GitLab Bot 🤖Setting devopscreate based on groupsource code.
added devopscreate label
unassigned @iamphill
- Resolved by Pedro Moreira da Silva
@annabeldunstone do you have the bandwidth to review this UX change?
assigned to @pedroms and unassigned @annabeldunstone
- Resolved by Pedro Moreira da Silva
@jboyson great improvement!
I think this change is fair. However, we need to mitigate the display of the diff background colors around the corners of the file header (especially when using more contrasting syntax highlighting themes):In order to do that, I tried commenting out the following styles, which give this final look to the file header (notice the not so rounded corners, but I think that's a reasonable compromise):
.diff-file .file-title, .diff-file .file-title-flex-parent { // border-top-left-radius: 4px; // border-top-right-radius: 4px; } .file-title-flex-parent, .file-holder .file-title-flex-parent { // border-top: 1px solid #e5e5e5; // border-radius: 4px 4px 0 0; } .file-holder { // border-top: 0; }
What do you think?
unassigned @pedroms
assigned to @pedroms
added 452 commits
-
e11ca14d...7a6fee09 - 449 commits from branch
master
- 2e70233a - Remove stickyMonitor from diff_file_head.vue
- aded40bb - Fix static analysis
- b0362db1 - Fix diff header background bleed
Toggle commit list-
e11ca14d...7a6fee09 - 449 commits from branch
added 65 commits
-
b0362db1...69fbeb10 - 62 commits from branch
master
- cc68f3b3 - Remove stickyMonitor from diff_file_head.vue
- 8b99bf05 - Fix static analysis
- 05e00220 - Fix diff header background bleed
Toggle commit list-
b0362db1...69fbeb10 - 62 commits from branch
- Resolved by Justin Boyson
unassigned @pedroms
added 233 commits
-
05e00220...d5c142b6 - 230 commits from branch
master
- e7a46101 - Remove stickyMonitor from diff_file_head.vue
- 2b63d6db - Fix static analysis
- f2d9fd5f - Fix diff header background bleed
Toggle commit list-
05e00220...d5c142b6 - 230 commits from branch
@pslaughter maintainer review please.
- Resolved by Paul Slaughter
- Resolved by Paul Slaughter
mentioned in issue #39145 (closed)
mentioned in commit 74abc282
Amazing @jboyson
- this is so good.mentioned in commit 7816adee
mentioned in issue #210024