-
Notifications
You must be signed in to change notification settings - Fork 140
add configuration possibilities in dashboards #147
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
base: master
Are you sure you want to change the base?
Conversation
format tables as images
implement simple method to store custom configuration for each dashboard
wilg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting solution to adding metadata for individual commands. I'm into it! I have a couple suggestions, and it looks like there are some TypeScript warnings and linter failures that need to be resolved.
I'd run yarn lint-fix to clean up anything that can be autofixed, and then work through the other issues by running yarn test.
Dockerfile
Outdated
| @@ -1,4 +1,4 @@ | |||
| FROM node:6.10.3-alpine | |||
| FROM node:8.11.4-alpine | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the node version change necessary? It doesn't seem like it. I would avoid changing this otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently checking why I changed that.
src/looker.ts
Outdated
| command.config = JSON.parse(dashboard.description); | ||
| command.description = command.config.description | ||
| }catch(e) { | ||
| console.warn("dashboard description is not valid json or starts with {") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think anybody will see these console warnings. Perhaps this should be surfaced in the help command instead.
src/looker_client.ts
Outdated
|
|
||
| newConfig.headers = _.extend(newConfig.headers, requestConfig.headers || {}) | ||
|
|
||
| console.log(newConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary console.log
| @@ -46,59 +46,93 @@ | |||
| string-format-obj "^1.0.0" | |||
| through2 "^2.0.0" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you did not change any dependencies, there should not be any changes to the yarn.lock file. Perhaps you inadvertently ran yarn update or something? We'll need to revert this before merging.
also switch code blocks to nicen up help message - remove console.log
…n the old version I think)
- test the old node version again (had problems with showing tables i…
|
thanks for the review 👍 |
|
will fix the tests. |
the basic format options offered by stack are limiting the table visualization.
Using images is a easy workarround.