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

Add "Updated At" to every core widget (with toggle) #84

Open mackenza opened 7 years ago

mackenza commented 7 years ago

I was speaking with @davejlong on Gitter and we chatted about a suggestion I had that we add an "Updated At" line to the bottom of each core widget.

The functionality to get the updated at datetime is already in the js helper library, so it would just be a case of making the line and class appear in the JSX and SCSS (respectively) for a widget.

Also, I would like to make it a flag, on by default. Most times you would want to see how fresh the data is, but sometimes not. Like if you had static text in a text widget, you wouldn't bother probably.

If this is an acceptable plan, I am happy to submit a PR for it. I have already started but wanted to socialize the idea in an issue before a PR out of the blue ;)

zorbash commented 7 years ago

Currently Kitto ships with:

A use case I can think of for adding updated-at to the image widget is for graph images retrieved from a service like Graphite but such services tend to have a way to embed update frequency info in the image (for example Graphite render API accepts title).

mackenza commented 7 years ago

agreed with the above. I think it makes sense for it to be toggle-able (not a real word...lol), though. Do you agree?

zorbash commented 7 years ago

Let's try to add it as an opt in field.

mackenza commented 7 years ago

before I submit a PR, let me run the code I am considering by you:

In dashboard:

 <div class="widget-welcome"
           data-widget="Text"
           data-source="phrases"
           data-title="Hello"
           data-text="This is your shiny new dashboard."
           data-moreinfo="Protip: You can drag the widgets around!"
           data-showupdated="1">
 </div>

In the widget code:

showUpdated() {
    if (!this.props.showupdated) { return ""; }

    return updatedAt(this.state.updated_at);
  }

  render() {
    return (
      <div className={`${this.props.className} ${this.status()}`}>    
        <h1 className="title">{this.state.title || this.props.title}</h1>
        <h3>{this.state.text || this.props.text}</h3>
        <p className="more-info">{this.state.moreinfo || this.props.moreinfo}</p>
        <p className="updated-at">{this.showUpdated()}</p>
      </div>
    );

So it's off by default. Possible values for data-showupdated are:

I suggest in the sample dashboard we would set at least one widget with the showupdated as shown in order to demonstrate the feature.

I would add this functionality to all the current widgets that have updated-at shown.

mackenza commented 7 years ago

changes can be viewed and tested using https://github.com/mackenza/devdash_kitto/tree/optional-updated-at if you want

zorbash commented 7 years ago

updated-at will be visible by default, to hide it an updatedAt property can be used. Have a look at https://github.com/kittoframework/kitto/blob/master/installer/templates/new/widgets/number/number.js#L39 We should use "off" to disable it for consistency.

When the user has decided not to show the updated-at I don't see a reason to include the <p> element. We could have an helper function returning the updated-at element.

Function should be placed in: https://github.com/kittoframework/kitto/blob/master/installer/templates/new/assets/javascripts/helpers.js

I'm thinking of something like the snippets below:

// In file: helpers.js
export const Components {
  UpdatedAt: function() {
    if (this.props.updatedAt == "off") { return; }
    <p className="updated-at">{updatedAt(this.state.updated_at)}</p>
  }
}
// In file: widgets/text/text.js
import {Components} from 'helpers.js';

class Text extends Widget {
  render() {
    return (
      <div className={`${this.props.className} ${this.status()}`}>    
        <h1 className="title">{this.state.title || this.props.title}</h1>
        <h3>{this.state.text || this.props.text}</h3>
        <p className="more-info">{this.state.moreinfo || this.props.moreinfo}</p>
        <Components.UpdatedAt updatedAt={this.props.updatedAt} />
      </div>
    );
   }
}

After the implementation is merged, we should as you say change the demo dashboard to contain an example of this.

mackenza commented 7 years ago

thx for reviewing

I had proposed updated-at being on by default, but I thought you had decided otherwise by your comment

Let's try to add it as an opt in field.

which I took as meaning you wanted it off by default and the author would need to opt in to have it shown. If I misread that, great, I thought it should be on anyway ;)

as for the returning of the whole

line from the helper (nice catch on the fact it should be in the helpers.js file, I forgot about that), I thought I had tried that and got some strange results... but I may have been returning "" rather than nothing like you are. I will test this out.

zorbash commented 7 years ago

In my https://github.com/kittoframework/kitto/issues/84#issuecomment-272554132 comment I was referring to the updated-at field in the image widget, there it made sense to me to have it opt in.

In order not to have conflicting conventions, I think it's better to have have it opt out everywhere using a common UpdatedAt component (even though I still feel most people will have to disable it in the image widget).

Keep in mind that I didn't try any of the snippets I shared for the component and they are mere samples of how I imagine the feature to be implemented. If you get more strange results or need any aid on this we should discuss it on Gitter.