Skip to content

add configuration possibilities in dashboards#147

Open
emoods wants to merge 20 commits intolooker:masterfrom
emoods:master
Open

add configuration possibilities in dashboards#147
emoods wants to merge 20 commits intolooker:masterfrom
emoods:master

Conversation

@emoods
Copy link
Copy Markdown

@emoods emoods commented Aug 23, 2018

the basic format options offered by stack are limiting the table visualization.
Using images is a easy workarround.

Copy link
Copy Markdown
Contributor

@wilg wilg left a 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.

Comment thread Dockerfile Outdated
@@ -1,4 +1,4 @@
FROM node:6.10.3-alpine
FROM node:8.11.4-alpine
Copy link
Copy Markdown
Contributor

@wilg wilg Aug 29, 2018

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.

Copy link
Copy Markdown
Author

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.

Comment thread 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 {")
Copy link
Copy Markdown
Contributor

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.

Comment thread src/looker_client.ts Outdated

newConfig.headers = _.extend(newConfig.headers, requestConfig.headers || {})

console.log(newConfig);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unnecessary console.log

Comment thread yarn.lock
@@ -46,59 +46,93 @@
string-format-obj "^1.0.0"
through2 "^2.0.0"
Copy link
Copy Markdown
Contributor

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.

pr12312 and others added 6 commits September 5, 2018 09:24
  also switch code blocks to nicen up help message
- remove console.log
- test the old node version again (had problems with showing tables i…
@emoods
Copy link
Copy Markdown
Author

emoods commented Sep 5, 2018

thanks for the review 👍

@emoods
Copy link
Copy Markdown
Author

emoods commented Sep 5, 2018

will fix the tests.

@emoods emoods changed the title add option to print tables as pictures add configuration possibilities in dashboards Sep 5, 2018
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