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

More links, dark theme #9

Merged

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Oct 28, 2019

My suggestion for a dark theme was very popular (3 upvotes just from @DasSkelett alone!), so here it is, borrowed from Youtube via developer tools.

Also when you hover a mod cell more hyperlinks appear to make it more convenient to get to the netkan's commit history and the .ckan files' folder. These will be accompanied by other links if/when we start setting them into the status objects (trivial to do in the Indexer if we decide to go that route): Homepage, SpaceDock, Repository, Curse, Bug Tracker.

image

Compilation and testing cheat sheet:

  1. Temporarily edit src/javascript/app.jsx to change http://status.ksp-ckan.space/status/netkan.json to netkan.json
  2. npm install
  3. npm run build && ( cd dist; rm netkan.json; wget http://status.ksp-ckan.space/status/netkan.json && python3 -m http.server 5000 )
  4. Open in browser: http://127.0.0.1:5000/

@DasSkelett
Copy link
Member

DasSkelett commented Oct 28, 2019

npm complained about security issues with css-loader (and others, but those are only 'low').

# Run  npm install css-loader@3.2.0  to resolve 2 vulnerabilities
SEMVER WARNING: Recommended action is a potentially breaking change
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Denial of Service                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ js-yaml                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ css-loader                                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ css-loader > cssnano > postcss-svgo > svgo > js-yaml         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/788                             │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Code Injection                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ js-yaml                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ css-loader                                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ css-loader > cssnano > postcss-svgo > svgo > js-yaml         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/813                             │
└───────────────┴──────────────────────────────────────────────────────────────┘

npm install css-loader@3.2.0 worked, but with the following output:

npm WARN ajv-keywords@3.4.1 requires a peer of ajv@^6.9.1 but none is installed. You must install peer dependencies yourself.
npm WARN css-loader@3.2.0 requires a peer of webpack@^4.0.0 but none is installed. You must install peer dependencies yourself.
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@1.2.9 (node_modules/fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@1.2.9: wanted {"os":"darwin","arch":"any"} (current: {"os":"linux","arch":"x64"})

Updating ajv to ajv@^6.10.2 appears to work. I can still scroll, search, click on links ;)

If I understand correctly, webpack is the thing our site is built on primarily, so updating the major version their could break some stuff.
I'll try nevertheless 😎

It's to bundle all the js source together for development. Could still break things. Let's see.


Regarding your code changes, they work fine as far as I can judge.

@HebaruSan
Copy link
Member Author

What kind of security? As in, they can hack the code running in their own browser? 😕

@DasSkelett
Copy link
Member

I have now idea. But it's nasty red.

With those changes, it still works fine from what I can tell, but it's reduced to only "low security warnings":

diff --git a/package.json b/package.json
index ccb82ab..23e456a 100644
--- a/package.json
+++ b/package.json
@@ -10,12 +10,12 @@
   "keywords": [],
   "dependencies": {
     "@babel/preset-react": "^7.6.3",
-    "ajv": "^5.5.2",
+    "ajv": "^6.9.1",
     "bootstrap": "^3.4.1",
-    "css-loader": "^0.28.11",
+    "css-loader": "^3.2.0",
     "fixed-data-table": "^0.6.5",
     "jquery": "^3.4.1",
-    "moment": "~2.18.1",
+    "moment": "^2.24.0",
     "rc-menu": "^7.5.3",
     "react": "^15.6.2",
     "react-dom": "^15.6.2",
@@ -33,7 +33,7 @@
     "@babel/plugin-proposal-object-rest-spread": "^7.6.2",
     "@babel/preset-env": "^7.6.3",
     "babel-loader": "^8.0.6",
-    "html-webpack-plugin": "^2.30.1",
+    "html-webpack-plugin": "^3.2.0",
     "webpack": "^3.12.0"
   }
 }

But the whole JS ecosystem is really foreign to me, so if you say we shouldn't update those packages, I'm totally fine with it.

@HebaruSan
Copy link
Member Author

It doesn't report this for you after that?

npm WARN css-loader@3.2.0 requires a peer of webpack@^4.0.0 but none is installed. You must install peer dependencies yourself.

Updating webpack to 4.x gave me syntax errors in webpack.build.config.js, the format apparently changed and I have no idea how to update it.

I'd rather not interact with the catastrophe that is node-js packaging more than we have to. None of this code runs on a server, so there are no security concerns here.

@DasSkelett
Copy link
Member

Somehow it didn't during install, but it does if I do

$ npm ls webpack
NetKAN-Status@0.0.0 /home/kilian/git/NetKAN-status
└── UNMET PEER DEPENDENCY webpack@3.12.0
npm ERR! peer dep missing: webpack@^4.0.0, required by css-loader@3.2.0

I'd rather not interact with the catastrophe that is node-js packaging more than we have to. None of this code runs on a server, so there are no security concerns here.

If we can agree that you won't do another PR to this repository where I'd have to touch npm again in the near future, I'm totally with you ;)

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Don't have the patience to dive into JavaScript now, and checking with all those libraries. No code review from me today.

It works, that shall be enough!

@techman83
Copy link
Member

We really need some CI/CD on this repo. Then we can automate all the madness that is NPM.

@techman83
Copy link
Member

Are you all happy with this PR @HebaruSan ? Is the build process something we can push through CI/CD easy enough?

@HebaruSan
Copy link
Member Author

Hmm, I should probably refactor the stylesheet stuff to be more in line with how React does styling. Maybe another day or two for that.

But I don't think the build process has changed. It should still be whatever it was when you created this repo. Unfortunately I don't know how to go about automating it.

@HebaruSan HebaruSan force-pushed the feature/more-links-and-dark-theme branch from f39dc7e to 68aeea5 Compare November 5, 2019 19:26
@HebaruSan
Copy link
Member Author

OK, now I think I'm happy with this.

  • Moved stylesheet to a css file
  • Errors are now red
  • Sorted the resource links
  • Resource links animate in and out
  • Two rows of links can fit in a cell
  • Rows have a secondary sort by id

@techman83
Copy link
Member

But I don't think the build process has changed. It should still be whatever it was when you created this repo. Unfortunately I don't know how to go about automating it.

I whipped this up in an afternoon because I wanted to learn a little react and needed a UI for the status information that was being generated. It was also before I did a lot of end-to-end automation.

They're static files, so it should be a case of build and copy to S3. So I'll sort that out after work. As for security, the scope for problems is probably limited, though possibly some cross site stuff? I haven't looked into the specific libraries and what the relevant attack vectors are.

This won't stop me from merging this, but something we should think on:

  • Is dark mode optional (I find the whole obsession odd, but I don't judge it)
  • Red mixed with dark colour schemes can be a problem for people who are colour blind.

@HebaruSan
Copy link
Member Author

  • Added a button to toggle dark/light (default is light)
  • Errors no longer red

@techman83 techman83 force-pushed the feature/more-links-and-dark-theme branch from be12039 to c03e1f2 Compare November 6, 2019 09:59
@techman83
Copy link
Member

In theory that should work, I'm guessing it's not running the deployment steps as they don't originate from the KSP-CKAN/NetKAN-status repo. Which is sane. Squashed down the commits to what I think should be good to go!

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.

3 participants