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

slack: Generalize theming #61328

Merged
merged 1 commit into from Jun 16, 2019
Merged

slack: Generalize theming #61328

merged 1 commit into from Jun 16, 2019

Conversation

NeQuissimus
Copy link
Member

Split out dark theme
Fixes #61155

Motivation for this change

#61155

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@joachifm
Copy link
Contributor

Not too important but this seem like it could be made fixed-output. Maybe consider making it local-only, as well.

@NeQuissimus
Copy link
Member Author

I do not know how to do that :)

@joachifm
Copy link
Contributor

Oh well, it's not terribly important. It'd just be an optimization.

Anyway, if you're interested in how to do it:

In general, creating a fixed-output drv amounts to declaring the output checksum, as in

let # for syntax highlighting
outputHashMode = "recursive"; # or "flat"
outputHashAlgo = "sha256";
outputHash = "....";

Since the drv in this case is supposed to produce an output containing a single file from a single file source, it'd be neater if you could flatten the whole thing into a single expr. I expect you could do that with fetchurl + postFetch. See https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/fetchurl/default.nix for the specifics.

In general, to make it local-only, you'd add attrs

let # for syntax highlighting
preferLocalBuild = true;
allowSubstitutes = false;

(This would be unnecessary with fetchurl).

Split out dark theme
Fixes NixOS#61155
@NeQuissimus NeQuissimus merged commit 66add9f into NixOS:master Jun 16, 2019
@NeQuissimus NeQuissimus deleted the slack-dark branch June 16, 2019 13:53
cat <<EOF >> $out/lib/slack/resources/app.asar.unpacked/src/static/ssb-interop.js
document.addEventListener('DOMContentLoaded', function() {
let tt__customCss = ".menu ul li a:not(.inline_menu_link) {color: #fff !important;}"
$.ajax({
url: '${darkModeCssUrl}',
url: '${theme}/theme.css',
Copy link
Contributor

@Gonzih Gonzih Jul 18, 2019

Choose a reason for hiding this comment

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

@NeQuissimus hey, sorry, but how is this suppose to work now?
This would expand for a new version of slack-dark package to this url in AJAX call.

https://myslackgroup.slack.com/nix/store/wcpbpwp161w5gvlchi2nnvra60l1dlzw-slack-theme-black/theme.css

This would never work AFAIK. It might work with file:// prefix, but from what I remember electron will always block AJAX calls to local filesystem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also since url now has postfix of /theme.css always I would be unable to pass my own custom URL as a workaround sadly. :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Created a fix for this #65156

@Gonzih Gonzih mentioned this pull request Jul 20, 2019
10 tasks
Gonzih added a commit to Gonzih/nixpkgs that referenced this pull request Jul 20, 2019
angristan pushed a commit to angristan/nixpkgs that referenced this pull request Jul 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slack: dark-mode introduces XSS vulnerability
3 participants