Refined-github: Feature specific CSS: Coupling CSS to a feature script

[wsmd]

This is a not Github-related feature request. The goal of this RFC is to discuss an idea related to code implementation of Refined Github.

Feature specific CSS: Coupling CSS to a feature script

Summary

Currently, all CSS live in one place: content.css. Feature-specific CSS is not tied to the feature itself. In some situations, especially when a feature requires both CSS and JS, it can be very handy if there is a way for a script to inject custom CSS that is strictly related to the said script. Something like this:

.
└─ features/
   ├─ add-something-cool.css
   └─ add-something-cool.js

Basic example

// /features/sticky-file-headers.js

import select from 'select-dom';
import {injectCSS} from '../lib/utils';
import styles from './sticky-file-headers.css'

export default function () {
  // when this feature is enabled it will inject some CSS on the page
  injectCSS(styles);
  // Code could also use styles this feature injected on the page
  for (const el of select.all('.file-header .tooltipped')) {
    // do something...
  }
}

/* /features/sticky-file-headers.css */

/* cool styles or global overrides */
.pull-request-tab-content .diff-view .file-header {/* ... */}

Motivation

Refined Github makes file headers sticky throughout Github, which is a very convenient feature! The feature itself is done purely in CSS, so it _does not_ live under the /features directory. It lives inside the content.css file.

There has been a long history of issues that this feature has caused related to tooltips within file headers. This also resulted in some inconsistencies as Github continued to add more tooltips that Refined Github does not account for. I attempted to resolve this particular issue in #1581, but the solution required some JS code to be added.

My first attempt was to add a script called fix-sticky-file-header-tooltips under /features. This approach didn't feel quite right because _this isn't necessarily a "feature", but rather a fix for another feature_ – the sticky file-header, that is.

@bfred-it suggested that we may want to have a feature script dedicated entirely for sticky headers. I like this idea very much! but there is a catch involved! _The feature would still require both CSS and JS!_

So the challenge here is:

• A) all CSS-based features live in one place
• B) there is no easy way to tie CSS to the script of a specific feature.

In situations like this, it can be very handy to have a way for a script to inject custom CSS that is strictly tied to the said script.

Something like this:

.
└─ features/
   ├─ add-something-cool.css
   └─ add-something-cool.js

Detailed design

The main idea behind this design is allowing feature scripts to inject CSS to a page.

Refined Github already does something similar by injecting custom CSS that the user specifies.

This is possible using /lib/utils.js#injectCustomCSS:

https://github.com/sindresorhus/refined-github/blob/f8abd1e5d7c9d3b45bf3f49e58244e01b22938ca/source/libs/utils.js#L154-L160

That helper could be refactored to accept any custom CSS string we pass it, not just from options. Feature scripts could leverage that helper to inject any CSS string to the pages on which they are enabled.

Feature scripts could have adjacent CSS files that they import and inject on the page on-demand.

This can be done with the help of the Webpack loader: raw-loader which resolves CSS files into actual strings.

Alternatives

Manually Styling Elements

// /features/add-awesome-feature.js
import select from 'select-dom';

export default function () {
  const fileHeader1 = select('.pull-request-tab-content .diff-view .file-header');
  fileHeader1.style.position = 'sticky';
  fileHeader1.style.top = '59px';
  fileHeader1.style.zIndex = 10;

  const fileHeader2 = select('.file .file-header');
  fileHeader2.style.position = 'sticky';
  fileHeader2.style.top = 0;
  fileHeader2.style.zIndex = 10;
}

Why should we not do this?

• Inline styles!
• Very verbose and difficult to work with

Template Strings

// /features/add-awesome-feature.js

import { injectCSS } from '../lib/utils';

const styles = `
  .awesome-thing {
    color: red;
  }
  .dropdown-menu {
    opacity: 0.5;
  }
`;

export default function () {
  injectCSS(styles)
}

Why should we not do this?

• Styles cannot be linted
• No syntax highlighting or editor/ide css-specific features
• Feature scripts become polluted with CSS template strings

More Advanced Webpack Loaders

Such as css-loader/style-loader.

Why should we not do this?

CSS will be injected automatically on import and cannot be injected congenitally.

For example:

import './add-awesome-feature'; // will be injected automatically upon importing

vs

import styles from './add-awesome-feature';
import {injectCSS} from './libs/utils';

export default function () {
  if (somethingIsTrue) {
    injectCSS(styles);
  }
}

Drawbacks

Injecting multiple instances of the same stylesheet

One of the drawbacks I could think of right now is the possibility of injecting multiple instances of the same stylesheet on the same page.

This may happen due to the nature of how some pages transition. Some pages are loaded via Ajax (while persisting the document head). Meaning, enableFeature() may get called multiple times, which will result in multiple calls to injectCSS of the same feature.

To get around this issue, we could add some caching mechanism. Consider the following pseudo-ish code:

const stylesheetCache = new Map();
const injectCSS = css => {
  const sheetId = hash(css)
  if (!stylesheetCache.has(sheetId)) {
    const stylesheet = <style>{css}</style>;
    stylesheetCache.set(sheetId, stylesheet);
    document.head.appendChild(stylesheet);
  }
}

Proof of concept

Unresolved questions

How can we remove styles from the page?

I have not given this a lot of thought yet. For now, I would say that this isn't currently an issue or a concern since Refined Github currently loads the entire content.css on all pages regardless if a feature is needed or not.

If truly a concern, it may be possible to resolve this with the help of the caching mechanism described above.

[wsmd]

A proof of concept for this idea can be found here: https://github.com/wsmd/refined-github/pull/1 That PR also include code from #1581

[busches]

Another alternative would be to add a new class in the JS, e.g. rgh-sticky-header and then reference that in addition to the current CSS selector.

[fregante]

That’s what we do here, yes:

https://github.com/sindresorhus/refined-github/blob/master/source/features/hide-useless-newsfeed-events.js

[wsmd]

Thanks for the suggestion! I'll definitely consider that for #1581 👍

[fregante]

I think the classes (the way I+@busches mentioned) are an lightweight and low-effort way to achieve this without overthinking it.

[wsmd]

Thanks for the feedback, y'all!

I can absolutely see how this can be added/unnecessary complexity; it's something that I should've listed as a drawback! Looks like the majority is in favor of using classes so I'm closing this issue for now.