text widget changes (related to #80)

kittoframework / kitto

Kitto is a framework for interactive dashboards written in Elixir

http://kitto.io/dashboards/sample

MIT License

955 stars 58 forks source link

text widget changes (related to #80) #82

Closed mackenza closed 7 years ago

mackenza commented 7 years ago

this is a cleaner PR than #80

The biggest change from that PR other than cleaning up based on comments was that the application.scss already had a facility for data-driven status changes and I got rid of my custom style concept in favour of using the already built-in capability.

So now in the job you can pass:

...
widgetStatus: "status-warning" // or "status-danger"

and the widget will flash red or yellow. @Zorbash - I notice this application.scss defines warning as red and danger as yellow. I feel like that's not intentional and they should be reversed. Do you want a PR for that?

coveralls commented 7 years ago

Coverage remained the same at 87.162% when pulling 057a30d0cd0b0e1a181b5fd3f507295fa2c256a0 on mackenza:text-widget-pr into ad560015d71a904d57ced912e54b333f1e743e95 on kittoframework:master.

mackenza commented 7 years ago

if we are good with the status thing, it should probably be added to all/most of the core widgets as it's common functionality defined cross-widget in application.scss

mackenza commented 7 years ago

I don't think ebert is behaving correctly... it always finds 84 issues and then times out after 15 mins.

zorbash commented 7 years ago

Having danger as yellow and warning as red is intentional and is actually originating from dashing (see: https://github.com/Shopify/dashing/blob/v1.3.50/templates/project/assets/stylesheets/application.scss#L15).

mackenza commented 7 years ago

Having danger as yellow and warning as red is intentional and is actually originating from dashing (see: https://github.com/Shopify/dashing/blob/v1.3.50/templates/project/assets/stylesheets/application.scss#L15)

I see that... doesn't make it right, though ;)

It is pretty standard convention that warning is yellow and error/danger/bad is red. I propose it get changed.

coveralls commented 7 years ago

Coverage remained the same at 87.162% when pulling 2b2cd223ee1ab42db171b9c148aa900ba2dc967c on mackenza:text-widget-pr into ad560015d71a904d57ced912e54b333f1e743e95 on kittoframework:master.

mackenza commented 7 years ago

how does one update wiki pages? It's not part of the PR is it?

zorbash commented 7 years ago

@mackenza I think there's no way to submit a PR for the wiki repo on GitHub. I'll update them appropriately myself after it get's merged.

zorbash commented 7 years ago

Concerning the colours for warning/danger i concur with you, but discuss any changes in a separate PR.

coveralls commented 7 years ago

Coverage remained the same at 87.162% when pulling b36eaac4949666f75c5f7814200414b842210e73 on mackenza:text-widget-pr into ad560015d71a904d57ced912e54b333f1e743e95 on kittoframework:master.

mackenza commented 7 years ago

that's what I get for coding while my ass froze watching my son play hockey... it actually wasn't even calling the function properly let alone generating the proper output. ;)

fixed.

zorbash commented 7 years ago

Squashed and cherry-picked as 9bb44ba6661a8126a71fd5cb4e8fff8156623b85 Thanks a lot @mackenza :+1:

zorbash commented 7 years ago

Ebert has finished reviewing this Pull Request and has found:

2 possible new issues (including those that may have been commented here).

But beware that this branch is 2 commits behind the kittoframework:master branch, and a review of an up to date branch would produce more accurate results.

You can see more details about this review at https://ebertapp.io/github/kittoframework/kitto/pulls/82.