#132 Add sticky header option

[bendera]

Issue Number / Link: #132

Summary of the issue:
Implemented the sticky header feature

Description outlining how this pull request resolves the issue:

• Added the stickyHeader configuration property. It's a boolean property and its default value is true. (Feel free to suggest modifications for the property name or the description, I'm not a native English speaker.)
• A "sticky" class will be added to the "#controls" and the "#tableColHeaders" element if the stickyHeader is enabled. CSS has been modified too.
• config.test.ts has been updated

[mhutchie]

Thanks for raising this pull request, it’s greatly appreciated!

When the feature request was first opened, I had a look into what would be involved to implement it. After having a quick look at the file changes, I’m sure additional work will be required to handle various other aspects of the UI. The reason why i hadn’t already implemented this feature request was because of all these other aspects that were complicated, and at the time would have required me to put a lot more thought to be put into them. In saying that, what you’ve done is the core foundation work that’s required. It should be easy to add a couple of extra changes to get this feature request implemented! Great work!

I’m currently working on the last feature for the next release (Git Graph 1.27.0), which I have to release this coming weekend. I’ve still got to do pre-release testing after this last feature is implemented before 1.27.0 can be released. I have to prioritise these commitments I already have, before I can take a proper look and this pull request.

I’ll aim to take a proper look at your changes this weekend after the release, and see what else is required (and we can coordinate getting this implemented - I’d be happy to help). From what I can see, everything you’ve already done looks great!

[mhutchie]

Would you be able to comment on the feature request, so I can assign it to you?

[bendera]

Yes, sure. Done.

[mhutchie]

Hi @bendera,

Apologies for the delay in reviewing this, Git Graph 1.27.0 took longer than I had planned to get released. I'll be taking a look at this PR in the next few days.

[bendera]

No problem. :)

[mhutchie]

Hi @bendera,

I've just reviewed your changes, thank you so much for your work!

After trying out your changes, I can see that most of the implementation concerns / considerations I previously had around making the header sticky are working perfectly in what you've already committed. Great work!

I did notice two things that weren't quite right in my initial testing, it would be great if you could try to resolve them:

1. If the controls overflow onto a second line, the table header isn't sticky (even though it still has the correct class):
   • Here is a control bar that fits all on one line:
     
   • When the views horizontal width is reduced, and the controls overflow onto a second line, the table header isn't visible:
     
2. There is a #scrollShadow element that is positioned at the top of the view. When the view has scrollTop > 0, then it has the .active class, otherwise it doesn't (i.e. scrollTop === 0). It's a subtle detail that I implemented so the view is consistent with Visual Studio Code text editors etc., and is most noticeable at the boundary between the Git Graph Tab and the Git Graph View content. It should be located at the top-most transition to scrollable content - currently its always at the top of the view, when it should be beneath the table column headers when they're sticky and the view is scrolled.
   • When the view has scrollTop === 0, the tab flows into the top content (correctly):
     
   • However when the view has scrollTop > 0, #scrollShadow is currently at the top arrow, when it should be at the bottom arrow:
     
   • For reference, this is what it looks like when the controls are sticky - it has the effect that it looks like the view content has scrolled underneath the tabs:
     

I did my initial testing of this PR on Windows, so I'm not sure whether (1) occurs on all platforms, or just Windows. Regardless, I've found that I need to test major UI changes like this one on Windows, Mac, and Web Browser (Windows or Mac) in order to ensure it's cross-platform compatible.

[bendera]

Done. I had to modify the #scrollShadow element but I copied the styles from an editor instance. So, in theory it looks the same as the built-in shadow of the vscode.

[mhutchie]

Thanks @bendera for making those changes, they look perfect!

I've now included this feature in the version of the extension I use daily, so I can use it and see if I can spot anything else that needs to be addressed (I doubt there will be anything else). I do this for all features that make non-trivial changes to the UI. I'll also do my cross-platform testing of this feature.

I'll aim to have this done by the end of this weekend, merge this pull request, and publish it in the next beta release so everyone can use it. Thanks again!

[mhutchie]

Hi @bendera,

I've been using the sticky header for the last two days, and have noticed a few more things that need to be addressed (see below).

I've also done some minor refactoring, and made one correction (see the messages of my commits for more details).

• The z-index of the sticky headers needs to be adjusted, for example:
  • The dropdowns are behind the shadow (it should be above):
    
  • The context menu is in front of the sticky header elements (it should be behind):
    
• The Scroll Shadow sometimes appears in front of the scroll bar (I think when the sticky mode is active, the scroll shadow should be within #view instead of document.body).
  
• When the position of the context menu is being calculated, it should take the sticky header into account. For example, in the below image the context menu top offset should be positioned under the bottom of the sticky elements (as after the z-index of the context menu is corrected, the user wouldn't be able to see the top of it).
  
• When you implemented alignTableHeaderToControls to position the table column headers (respecting when the controls overflow onto multiple lines), I previously only mentioned one example that could cause it. It can also occur when:
  • The user transitions from having one repository in the workspace to having multiple in the workspace (or vice versa).
  • When the user switches repositories in a workspace:
    • The branches dropdown will change width to fit different length branch names.
    • The fetch button may be shown or hidden depending on whether the repository has any remotes.
  • There may be various other uncommon cases (e.g. the user changes the Visual Studio Code font size setting in a separate Editor Group).
  • As there are numerous triggers for the controls to overflow, I think it would be best to find a solution that avoids needing to calculate the height of the control bar.

It would be great if you can address some of these items.

[bendera]

Nice catches! I'm going to fix them in the next few days.

[bendera]

The Scroll Shadow sometimes appears in front of the scroll bar (I think when the sticky mode is active, the scroll shadow should be within #view instead of document.body).

I've tried to reproduce this but I couldn't. (Windows 10, Vscode 1.51.0)

I've made the context menu fixed, which solved the overlapping issues. Incidentally, its behavior is more similar to the vscode built-in context menu. I've created a different commit for this, so you can revert it if you don't like it.

All other issues are fixed.

[UPDATE]

I accidentally missed your last paragraph.

The user transitions from having one repository in the workspace to having multiple in the workspace (or vice versa).

In this case, the whole vscode window will be reloaded, won't be? Or are you referring to that when you add/remove a directory to an existing workplace?

In general, the table header position will be recalculated when the renderTable called or the window resized. The most error-proof solution would be that if the controls and the table header (a virtual table header) were in a wrapper element and the table in the other one. Possibly the table would get another hidden header (a real thead element) for accessibility purposes. And if you resize the virtual header cells the width values will be copied to the cells of the first row.

[mhutchie]

Apologies for not getting back to you, I just saw your reply now (I missed the email notification that was sent) see my new comment explaining what I just realised has occurred.

Thanks for making those fixes!

In this case, the whole vscode window will be reloaded, won't be? Or are you referring to that when you add/remove a directory to an existing workplace?

The Visual Studio Code Window is reloaded when switching from a standard workspace to a multi-root workspace. However, it is possible for repositories to be added and/or removed within the current workspace, without changing the number of workspace roots (e.g. the repositories are in subdirectories). If the user transitions from having one repository to multiple repositories (or vice versa), the "Repo" dropdown in the control bar is shown or hidden. This significant change in content length has the potential to cause an overflow in the control bar, meaning the table column headers would need to be repositioned.

The most error-proof solution would be that if the controls and the table header (a virtual table header) were in a wrapper element and the table in the other one. Possibly the table would get another hidden header (a real thead element) for accessibility purposes. And if you resize the virtual header cells the width values will be copied to the cells of the first row.

I think that this may possibly be the best solution, although I think it will be challenging to ensure the table column widths are always correct (as there's the default auto layout of columns, and the user can manually resize columns).

[mhutchie]

I just realised what occurred...

I found the email notification I received for your last comment in my inbox, and I remember reading it at the time now that I see it. However, it was sent when you posted your initial comment, which only stated that you'd fixed some of the changes I'd requested. As I knew there was still something else that you needed to do, I didn't open the PR on GitHub as I figured that the other complicated changes would come later. When you edited the comment to add your questions, I never got notified of them as GitHub doesn't seem to send emails when comments are edited.

At least this explains what occurred. I'm always extremely careful to read every email notification I get, so I was surprised when I saw your questions from a month ago that I hadn't responded to. Apologies again, I still should have checked up on the progress of this PR on GitHub sooner (instead of leaving it so long) - I'll be more cautious of this in the future to avoid similar delays.

[bendera]

It's my fault too. I could have asked for a status update.

Anyway. What do you think about the fixed context menu? Is it a good idea?

I gonna resolve conflicts today. And I will try that case with the multi-root workspace.

[mhutchie]

I've been having a look around how context menu's behave in a variety of different products. They appear to fall into a few categories:

1. Right click opens the context menu, prevents the view from scrolling until the context menu is closed. (e.g. Windows File Explorer, Microsoft Excel, Notepad++)
2. Right click opens the context menu, scrolling closes the context menu. (e.g. Google Chrome)
3. Right click opens the context menu, scrolling causes the content to scroll, but the context menu stays in the same position relative to the screen. (e.g. Visual Studio Code, Microsoft Word)
4. Right click opens the context menu, scrolling causes the content to scroll, and the context menu stays anchored relative to the element that was right clicked on. (e.g. Git Graph)

Options 1 - 3 are definitely easiest to implement, which is no-doubt why many products prefer them. Even so, I think Option 4 (the currently released behaviour in Git Graph) is the best option, as there is a strong advantage in being able to right click on an element, and the context menu stays anchored to the element regardless of whether the user scrolls the view before taking an action. For example, they may want to check some other information in the view before opening a dialog, without losing track of the element they wanted perform the action on. I've had feedback from other users who share the same sentiment, so I'd prefer if we tried to keep the existing behaviour if at all possible (even if it's slightly more complicated to achieve - I'd be happy to work on this). However, if it becomes too difficult to achieve, I'm definitely open to using the position: fixed approach, considering it simplifies things and is consistent with Visual Studio Code (as you mentioned).

Considering you've already changed the context menu to be fixed, let's keep it in for now. Once we're in a position to merge this PR, we can decide what we should do.

FYI: I've got some other context menu changes to implement in the next week, so that might also necessitate the switch to position: fixed as well.

[bendera]

Conflicts are resolved.

I've tried the multi-repo scenario and it works perfectly. That's how I tested it:

1. I've opened a single-root project. Then, I opened the Git Graph in a split editor view, the header broke into two lines.
2. I've added a new git-versioned directory to the workspace. The repo dropdown appeared and the header broke into three lines.

Are these the right testing steps?

[mhutchie]

Yes, Those steps will have set everything up that you need to trigger that scenario.

Although, now that you have a workspace with multiple repositories, you can just use the extension commands “Git Graph: Remove Git Repository...” & “Git Graph: Add Git Repository...” to remove and add back in a repository (to make it faster to test the transition while the Git Graph View stays visible).

[bendera]

Hi @mhutchie,
Do I have anything else to do?

[mhutchie]

Hi @bendera,

I was waiting to hear back on how you’re going with handling the many ways the controls can overflow (not just the ones I specifically mentioned as examples). How have you gone with this, as I can’t see that you’ve implemented it yet?

The most error-proof solution would be that if the controls and the table header (a virtual table header) were in a wrapper element and the table in the other one. Possibly the table would get another hidden header (a real thead element) for accessibility purposes. And if you resize the virtual header cells the width values will be copied to the cells of the first row.

I think that this may possibly be the best solution, although I think it will be challenging to ensure the table column widths are always correct (as there's the default auto layout of columns, and the user can manually resize columns).

[niccolomineo]

Hi, is this still being worked on? It would be great to see it implemented.

[bendera]

Well... It's almost done. There is a small issue: the scrollbar reaches the top of the page but it should finish under the header. It's not fixable but it doesn't affect usability. I couldn't find any other issue. I should test in different scenarios but I don't know all of the features of this extension and I can't test on Mac. I need a checklist or somebody who does this e2e testing for me. So, I've put aside this ticket. Yes, procrastination is my best friend.

Anyway, I've updated my forked branch and attached a build from it.
git-graph-1.31.0-alpha.0.zip

[mhutchie]

Hi @niccolomineo,

There are still some pending issues with the current implementation that means it’s not in a production-ready / broadly releasable state, as I previously mentioned. I believe one of the outstanding changes will require a fundamental code change. As with any feature, I only merge and release it once any outstanding issues have been addressed.

Unlike many other changes, due to the nature of this feature it would likely be as much work to implement & properly test a feature flag for it, as it would be to just fix the outstanding issues.

Once I put a feature in a beta release, I’m committed to including it in the next release of Git Graph. The best this feature can be released is in an alpha release, which has already been made available.

[gtnIIS]

Hi,
@mhutchie has already made his point so actually my question would be for @bendera if there is any news from your site on this issue? Are you still working on it?

[bendera]

I think this feature is production-ready. There is a small layout issue but it's not critical. I have been using my own build for a year which is forked from the latest stable release and works well. I only use the basic functions so I can't test the edge cases.

[gtnIIS]

Thanks for the reply, Adam. @mhutchie what's your current point of view on that? It's kinda sad to have a working and releasable feature sitting around, without being published.

[hansu]

One little thing that can be improved is the upper end of the scroll bar.
I think the scroll bar should end below the header and not be beside the header.

[hansu]

By the way, here you can get a version which includes this PR: https://github.com/hansu/vscode-git-graph/releases/tag/v1.31.0-beta.3.1

[niccolomineo]

@mhutchie any estimated time of merge of this? thanks

[bendera]

@mhutchie any estimated time of merge of this? thanks

It seems this project become abandoned. mhutchie doesn't have any activity for years.

[gtnIIS]

@niccolomineo @mhutchie hasn't been participating in this repo for a long time and plays the dead fish. Without any other maintainer being assigned this repo is also dead in the water...

of course in his linkedin profile he mentions to be still active mainting it... :-x

[gtnIIS]

who knows. he seems unresponsive to any try of contact. If you have linkedin pro, you can contact him there...

[mannfuri]

I think this feature is production-ready. There is a small layout issue but it's not critical. I have been using my own build for a year which is forked from the latest stable release and works well. I only use the basic functions so I can't test the edge cases.

thanks sir!
works perfectly!