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

Conversation

@larz0
Copy link
Member

@larz0 larz0 commented Jul 21, 2014

Doubled checked again after a PR was merged and realized I missed two spots while I was reviewing. This PR will fix the problem in the screenshots below:

Update: There are also fixes for #8491 and #8494.

screen shot 2014-07-21 at 12 57 06 pm

screen shot 2014-07-21 at 12 53 38 pm

@dangoor
Copy link
Contributor

dangoor commented Jul 21, 2014

Should that .inline-widget color be set somewhere in the core style sheets?

@larz0
Copy link
Member Author

larz0 commented Jul 21, 2014

@dangoor it only applies to the dark theme though.

@dangoor
Copy link
Contributor

dangoor commented Jul 21, 2014

OK

@larz0
Copy link
Member Author

larz0 commented Jul 21, 2014

@dangoor it's a temporary fix. We won't need it if it's using Dark Core UI.

@larz0
Copy link
Member Author

larz0 commented Jul 21, 2014

@dangoor I think we should take out the dark scrollbar styles out as well until the dark core UI is in place, what do you think?

screen shot 2014-07-21 at 2 21 56 pm

@peterflynn
Copy link
Member

@larz0 Seems like it could/should be scoped to only affect the editor area scrollbars. E.g. the sidebar already has different scrollbars (which are dark, narrower, and disappear on hover) and this blows them away, which is probably a bug even once we do have a fully-dark UI...

@larz0
Copy link
Member Author

larz0 commented Jul 21, 2014

@peterflynn cool I'll try and make the scrollbar styling editor-only in this PR.

@TomMalbran
Copy link
Contributor

The dark overwritten scrollbars are the one that should be scoped to the editor, and not the default ones, since we do want the win8/linux scrollbars in the dialogs and not the default ones.

@larz0
Copy link
Member Author

larz0 commented Jul 21, 2014

@TomMalbran @peterflynn I'm having a hard time trying to isolate the scrollbars.

@peterflynn
Copy link
Member

@larz0 You should be able to do something like #editor-holder ::-webkit-scrollbar-thumb, or maybe better yet .CodeMirror-vscrollbar::-webkit-scrollbar-thumb, .CodeMirror-hscrollbar::-webkit-scrollbar-thumb

@larz0
Copy link
Member Author

larz0 commented Jul 22, 2014

I tried that and I'm wondering if I had ran into a LESS bug.

@peterflynn
Copy link
Member

@larz0 Hmm... I think there used to be a LESS bug where those -webkit-prefixed things get dropped from the generated CSS, but that can't be true anymore since we've been storing them in 'brackets_scrollbars.less' for a while now.... But @MiguelCastillo's comment in #8481 (comment) seems to suggest there's still a problem with LESS.

@TomMalbran
Copy link
Contributor

@peterflynn I just figured it out. It is not an issue with Less, but an issue with how the Themes manager works and with the specificity required to overwrite the default styles. The big issue is that the Themes extensions extracts the scrollbar declarations and it re-adds them in a different style block when the custom scrollbars is on. The issues is that when doing this, it doesn't add the .platform-* which is required to make the dark custom scrollbars be at the same level of specificity as the default scrollbars and be able to overwrite the styles. I also tried inlining the .platform-* with ::-webkit-scrollbar-* and it didn't fix the issue.

@MiguelCastillo
Copy link
Contributor

@TomMalbran That's correct. That's why I was explaining that the order of operations needs to be swapped. The issue is that the scrollbar are extracted before they are processed by the LESS processor, which means that the platform-win & {} does not get converted to CSS... I am about to do a pull request anyways

@TomMalbran
Copy link
Contributor

@MiguelCastillo I actually also tried using .platform-win ::-webkit-scrollbar in the less file and it is still did not extracted it properly. So you might need to also change the regexp.

@MiguelCastillo
Copy link
Contributor

Yeah, that's where I am at right now. PITA. I was hoping to never have to touch that regex ever again :D

@MiguelCastillo
Copy link
Contributor

HA! Finally found where I mentioned that. #8481 Wrong issue

@TomMalbran
Copy link
Contributor

Oh yes. I saw that comment. But now I understand what you where saying. Hehe

@larz0
Copy link
Member Author

larz0 commented Jul 22, 2014

@peterflynn @dangoor should we merge this and make the scrollbar fix a separate PR? This definitely needs to go into 42.

@peterflynn peterflynn changed the title Negative areas of the selection triangle need to be transparent. Make negative areas of selection triangle transparent (& other fixes) Jul 22, 2014
@TomMalbran
Copy link
Contributor

@larz0 I just found out that the underline cursor used for overwrite is not working on the dark theme. It looks like you didn't added a style to overwrite it. https://github.com/adobe/brackets/blob/master/src/styles/brackets_codemirror_override.less#L197

Also, maybe we can remove some of the !important used on the cursors styles.

@larz0
Copy link
Member Author

larz0 commented Jul 22, 2014

Thanks @TomMalbran it's fixed now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct in thinking that this should actually be applied in the core stylesheet with the (not yet there) dark class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep you're right, these are temporary fixes. This was fixed in the dark core UI PR.

@dangoor
Copy link
Contributor

dangoor commented Jul 22, 2014

Good collection of changes. Merging

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.

6 participants