Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Larz/linux scrollbar#5083

Merged
jasonsanjose merged 13 commits intomasterfrom
larz/linux-scrollbar-sqaushed
Sep 6, 2013
Merged

Larz/linux scrollbar#5083
jasonsanjose merged 13 commits intomasterfrom
larz/linux-scrollbar-sqaushed

Conversation

@jasonsanjose
Copy link
Copy Markdown
Member

This is for #4884.

Squashed from #5082 for @larz0

@ghost ghost assigned jasonsanjose Sep 5, 2013
Comment thread src/styles/brackets.less Outdated
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There's a note in index.html for quiet-scrollbars.css from @njx about not being able to use this webkit specific syntax in less files. This seems to work fine though. Did something change?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't know about that. Did it get fixed? less/less.js#1152

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The difference is that these are applied to any element, while the quite scrollbars are applied to elements with a particular class. The issue @larz0 linked to, has a solution to use it in less:

.webkit-scrollbar-gray {
    &::-webkit-scrollbar {
        height: 4px;
    }
}

Should we have all the custom scrollbars together in a separate file?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Looks like it.

@jasonsanjose
Copy link
Copy Markdown
Member Author

@larz0 initial review complete

@TomMalbran
Copy link
Copy Markdown
Contributor

I think that the scrollbars will look a better with some spacing around them, which can be achieved with:

    ::-webkit-scrollbar-thumb {
        border-radius: 999px;
        box-shadow: 0 0 0 4px @custom-scrollbar-thumb inset;
        border: 2px solid transparent;
    }

    ::-webkit-scrollbar-thumb:window-inactive {
        box-shadow: 0 0 0 5px @custom-scrollbar-thumb-inactive inset;
    }

Note that the padding is added with transparent border.

I think that the PNG images in the quiet scrollbars when used to add a spacing, which can be achieved with this.

@jasonsanjose
Copy link
Copy Markdown
Member Author

It seems like we want these scrollbars to appear everywhere for Linux except the project panel. We want scrolling content in other areas like the extension manager dialog. Maybe the project panel should stop being an exception? At least for Linux?

@TomMalbran
Copy link
Copy Markdown
Contributor

I was able to fix the scrollbars issue here: https://gist.github.com/TomMalbran/6458648

Which is also using a 2px spacing.

@larz0
Copy link
Copy Markdown
Member

larz0 commented Sep 6, 2013

@TomMalbran I replaced all the scrollbar styles with your brackets_scrollbars.less. Could you test the custom scrollbar? I couldn't get it to show with .platform-mac.

Comment thread src/index.html Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

brackets_scrollbars is a less file. We could just remove this line and include it from brackets_shared.less.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alright cool I'll give that a try.

@TomMalbran
Copy link
Copy Markdown
Contributor

@larz0 no problem, happy to help.

I think we could also update the quiet scrollbars to get rd of that PNG?

@larz0
Copy link
Copy Markdown
Member

larz0 commented Sep 6, 2013

Alright I'll give that a try.

@TomMalbran
Copy link
Copy Markdown
Contributor

should be similar to the other ones, but with a different color. And since it looks like the quiet scrolbars use a 1px padding, maybe that should be used in the linux scrollbars too?

@larz0
Copy link
Copy Markdown
Member

larz0 commented Sep 6, 2013

Looks good on Mac, not sure if it's good on Windows also.

@TomMalbran
Copy link
Copy Markdown
Contributor

I can't see scrollbars anymore without the PNGs. You should add a box-shadow and border, in replace of the images, as it was done with the linux scrollbars.

I updated my gist https://gist.github.com/TomMalbran/6458648 and with those changes I can see it again. You can change any colors and sizes.

@larz0
Copy link
Copy Markdown
Member

larz0 commented Sep 6, 2013

@TomMalbran updated. Thanks!

@jasonsanjose
Copy link
Copy Markdown
Member Author

Looks good on mac, win and linux. Now all we need is the win8 style scrollbars! Nice work guys. Merging.

jasonsanjose added a commit that referenced this pull request Sep 6, 2013
@jasonsanjose jasonsanjose merged commit fd39c6d into master Sep 6, 2013
@jasonsanjose jasonsanjose deleted the larz/linux-scrollbar-sqaushed branch September 6, 2013 16:55
@peterflynn
Copy link
Copy Markdown
Member

@larz0 @TomMalbran I noticed something in this LESS code today while playing with some other scrollbar stuff: there's extra padding at the far end of the Linux scrollbar track (bottom of a vertical scrollbar, right side of a horizontal one). It looks like this was introduced in 7ba7240. Did we intend for the spacing to be asymmetrical like that? It makes it feel a little bit like you can't scroll all the way to the end...

@larz0
Copy link
Copy Markdown
Member

larz0 commented Mar 6, 2014

@peterflynn could you attach a screenshot? I can't remember…

@TomMalbran
Copy link
Copy Markdown
Contributor

It kind of was intentional since we might need to patch CodeMirror to fix it. The problem seems to be that CodeMirror can't calculate the space used by the scrollbars when use this custom ones. That means that it doesn't add the extra space at the bottom/right of the bars when there are both horizontal and vertical ones. So without the spaces added here the bars overlap on that corner, which is what is currently happening with the windows scrollbars.

We can fix this in CSS if we are able to know when we have both vertical and horizontal bars, which we could know if CodeMirror would add an extra class with this information.

@TomMalbran
Copy link
Copy Markdown
Contributor

You can see that here https://github.com/adobe/brackets/blob/master/src/styles/brackets_codemirror_override.less#L140 I had to set the width to 12px since CodeMirror gives them a width of 0px. Same with the height.

@peterflynn
Copy link
Copy Markdown
Member

@TomMalbran Interesting. Never noticed that odd overlap on Windows but now that you mention it I can indeed see that. I wonder if we should submit a CM bug on that, or a patch to provide a CSS class that disambiguates that case...

@larz0 Fwiw, here's the current appearance:
scrollbar-top
scrollbar-bot

@TomMalbran
Copy link
Copy Markdown
Contributor

We should submit a bug and ask marijnh if this is fixable in CM, and if it not, ask to add that class so we can fix it in Brackets.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants