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

List: additionalScrollHeight #78773

Merged
merged 3 commits into from Aug 9, 2019
Merged

Conversation

isidorn
Copy link
Contributor

@isidorn isidorn commented Aug 9, 2019

This PR adds the additionalScrollHeight option to the list and propagets it through the tree classes. This option allows the list clients to specify additional scrollHeight. I decided to make this a number, not a boolean since clients are in charge of the height of each element.

Here the explorer also starts using this options, specifying twice the element height.
The motivation for this is to tackle #1043 so it would be easier for users to create top level elements in the full explorer by scrolling all the way down.

I tried it out and I actually like how it feels.
Let me know what you think and thanks for the review.

@isidorn isidorn added file-explorer Explorer widget issues tree-widget Tree widget issues labels Aug 9, 2019
@isidorn isidorn added this to the August 2019 milestone Aug 9, 2019
@isidorn isidorn requested a review from joaomoreno August 9, 2019 09:27
@isidorn isidorn self-assigned this Aug 9, 2019
Copy link
Member

@joaomoreno joaomoreno left a comment

Choose a reason for hiding this comment

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

Not too happy about this either in code nor in the product. Here are some alternative ideas to tackle #1043:

  • Add additional context menu actions on files: New Sibling File, etc;
  • Add a Parent Folder submenu to the file context menu, containing all actions of its parent folder;
  • Tell users to right click on the scrollbar, it actually behaves as if you're right clicking the root. 😆

@bpasero
Copy link
Member

bpasero commented Aug 9, 2019

My 2 cents: we have discussed all those alternatives in length and they are not nice. I personally found this proposed solution to be one of the best so far and would go for it.

Please do not add a new menu entry. And please do not force everyone to have a root node.

@isidorn
Copy link
Contributor Author

isidorn commented Aug 9, 2019

@bpasero @joaomoreno and me just discussed this offline.
And decided to go with this PR and this approach. However we will decrease the empty space to be one element height, and not two.
Submlime does something similar

Screenshot 2019-08-09 at 14 44 43

@isidorn
Copy link
Contributor Author

isidorn commented Aug 9, 2019

After we selfhost on this for a couple of weeks we can revert it if we hate it, or we can tune the space at the bottom.

@bpasero
Copy link
Member

bpasero commented Aug 9, 2019

Cool!

@isidorn isidorn merged commit ab60600 into master Aug 9, 2019
@isidorn isidorn deleted the isidorn/listAdditionalScrollHeight branch August 9, 2019 13:28
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
file-explorer Explorer widget issues tree-widget Tree widget issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants