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

Fix #983 #1075

Closed
wants to merge 2 commits into from
Closed

Fix #983 #1075

wants to merge 2 commits into from

Conversation

the-j0k3r
Copy link
Member

@the-j0k3r the-j0k3r commented Feb 24, 2020

Or something of the sort See issue #983 for info

removed generator and added exclusion to some selectors

This could be a fix, or something of the sort. Havent touched it because its a can of worms, fixes here breaks elsewhere and half the other fixes just dont seem to stick around.

@the-j0k3r the-j0k3r changed the title Fix 983 Fix #983 Feb 24, 2020
@silverwind
Copy link
Member

silverwind commented Feb 24, 2020

Test images:

osm-school5

osm-school6

I don't like the white BG on the second one and I do believe the issue is essentially unfixable because the image in the issue was generated on the assumption it will render on a white background, so I'd lean towards closing #983 as "working as intended".

@the-j0k3r
Copy link
Member Author

Idk its broken if you cant see the images without interaction, or do I miss the point?

@silverwind
Copy link
Member

Maybe a 50% opacity white hover bg might work, similar to what we have on dedicated image pages.

@xt0rted
Copy link
Member

xt0rted commented Feb 24, 2020

I don't like the dark background on images either. I've been running this for awhile now.

.comment-body.markdown-body img:not(.emoji):not([data-canonical-src^="https://api.dependabot.com/"]):not([data-canonical-src^="https://dependabot-badges.githubapp.com/"]):not([data-canonical-src^="https://cla-assistant.io/"]):not([data-canonical-src^="https://cla.dotnetfoundation.org/"]):not([data-canonical-src^="https://cla.opensource.microsoft.com/"]) {
  background-color: #fff !important;
}

@the-j0k3r
Copy link
Member Author

the-j0k3r commented Feb 25, 2020

Maybe a 50% opacity white hover bg might work, similar to what we have on dedicated image pages.

I think this should work without user interaction i.e. hover over the image to see it.

If we supported some way of adding a stylus setting that would make this true per default and then optionally override with a hover hack, so users that dont like white bg can use the hover and users that want to see images dont use that setting.

Actually having said that, I can easily knock up a Override Style to do just that and then GitHub Dark can stay as is.

I'll also try @xt0rted 's CSS on this and come back when I have something.

@the-j0k3r
Copy link
Member Author

The main issue is that GitHub Dark has generated portions for this that cant be overwritten,

this is the sort of thing that cant be tied down to one rule. Can we remove the generator portions and exclude .markdown-body img in generator?

@silverwind
Copy link
Member

silverwind commented Feb 25, 2020

generated portions for this that cant be overwritten

Every generated rule can be overwritten and I think we should use the exclude list sparingly.

@the-j0k3r
Copy link
Member Author

the-j0k3r commented Feb 25, 2020

Every generated rule can be overwritten and I think we should use th exclude list sparingly.

Yea you would think so but if the gen stuff stays I cant use the override css.

GHD and the bellow CSS

/* ==UserStyle==
@name         GitHub Dark IMG Background Color
@namespace    StylishThemes
@version      0.0.6
@description  A GitHub Dark image background color user customizable style.
@author       StylishThemes
@homepageURL  https://github.com/StylishThemes/Feature-Override-Styles/
@supportURL   https://github.com/StylishThemes/Feature-Override-Styles/issues
@updateURL    https://raw.githubusercontent.com/StylishThemes/Feature-Override-Styles/master/github-dark-img-background.user.css
@preprocessor stylus
@var color backgroundClr "Background color" rgba(255, 255, 255, .9)
@var checkbox bgClrHack  "Background color hover hack" 0
==/UserStyle== */

@-moz-document regexp("^https?://((education|gist|guides|help|lab|launch-editor|resources|status|developer|support)\\.)?github\\.com/((?!generated_pages/preview).)*$"), domain("githubusercontent.com"), domain("graphql-explorer.githubapp.com"), domain("www.githubstatus.com") {
  if not bgClrHack {
    .comment-body.markdown-body img[data-canonical-src*="svg"],
    .comment-body.markdown-body img.emoji,
    .comment-body.markdown-body img[src*="raw.githubusercontent"],
    .comment-body.markdown-body img[src*="png"] {
      background-color: inherit !important;
    }
    .comment-body.markdown-body img:not([src*="raw.githubusercontent"]):not(.emoji):not([data-canonical-src*="dependabot"]):not([data-canonical-src*="githubapp.com"]):not([data-canonical-src*="cla-assistant.io"]):not([data-canonical-src*="cla.dotnetfoundation.org"]):not([data-canonical-src*="cla.opensource.microsoft.com"]) {
      background: backgroundClr !important;
    }
  }
  else {
    .comment-body.markdown-body img[data-canonical-src*="svg"],
    .comment-body.markdown-body img.emoji,
    .comment-body.markdown-body img[src*="raw.githubusercontent"],
    .comment-body.markdown-body img[src*="png"] {
      background-color: inherit !important;
    }
    .comment-body.markdown-body img:not([src*="raw.githubusercontent"]):not(.emoji):not([data-canonical-src*="dependabot"]):not([data-canonical-src*="githubapp.com"]):not([data-canonical-src*="cla-assistant.io"]):not([data-canonical-src*="cla.dotnetfoundation.org"]):not([data-canonical-src*="cla.opensource.microsoft.com"]):hover {
      background: backgroundClr !important;
    }
  }
}

there's something for each user

generic

So technically this PR isnt needed and the override style handles the can or worms the best way it can.

It wont cover 100% of cases but should be enough, ™️ famous last words

@silverwind
Copy link
Member

Looking good, I'd post is a addon style and link it in README.

@the-j0k3r the-j0k3r closed this Feb 25, 2020
@the-j0k3r the-j0k3r deleted the fix-983 branch February 26, 2020 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants