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

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

Open
mackenza opened this issue Jan 12, 2017 · 8 comments
Open

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

mackenza opened this issue Jan 12, 2017 · 8 comments

Comments

@mackenza
Copy link
Contributor

mackenza commented Jan 12, 2017

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
Copy link
Member

zorbash commented Jan 13, 2017

Currently Kitto ships with:

  • list ✔
  • meter ✔
  • number ✔
  • text ✔
  • clock (does not apply)
  • graph (see: graph widget thoughts #86)
  • image (it might look ugly to display updated-at over an image)

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
Copy link
Contributor Author

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
Copy link
Member

zorbash commented Jan 13, 2017

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

@mackenza
Copy link
Contributor Author

mackenza commented Jan 14, 2017

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:

  • don't include the element in the dashboard at all - Updated At: will be not shown
  • data-showupdated="" - Updated At: will not be shown
  • 'data-showupdated="<any non-empty string even 'false'"` - Updated At: will be shown

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
Copy link
Contributor Author

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

@zorbash
Copy link
Member

zorbash commented Jan 14, 2017

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
Copy link
Contributor Author

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
Copy link
Member

zorbash commented Jan 14, 2017

In my #84 (comment) 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.

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

No branches or pull requests

2 participants