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

Return value of the date knob is unclear #1489

Closed
simon360 opened this issue Jul 19, 2017 · 4 comments
Closed

Return value of the date knob is unclear #1489

simon360 opened this issue Jul 19, 2017 · 4 comments

Comments

@simon360
Copy link
Contributor

The documentation for the date knob is a bit sparse.

The date knob returns a UNIX timestamp as a string. That's a perfectly fine way to return a date/time, but it's one of many. It's especially confusing, because the knob accepts a new Date as input, but returns a string of a UNIX timestamp.

I'm happy to send a PR for this, but I wanted to open it up for discussion first, since the discrepancies between input and output seem a bit confusing, even with documentation. A different implementation may be desirable?

@simon360
Copy link
Contributor Author

A further breakdown of my concerns:

  • Accepting a new Date as input, but not returning a Date as output, feels like there's no consistent way of representing a datetime
  • A UNIX timestamp in JavaScript is usually represented as a Number; moment throws a warning when you provide a string of a timestamp to its constructor, so this implementation is inconsistent with some prior art.
  • My component accepts an ISO 8601 string as its input, because its underlying HTML element is a <time>, and that's what's accepted in the dateTime attribute of <time>.

I realize that not everyone will have the same experience, but all of the above factors combine in my case to result in the following component call in my story, just to pass a dateTime prop:

import React from 'react';
import DateTime from './DateTime';
import { date, withKnobs } from '@storybook/addon-knobs';
import moment from 'moment';

storiesOf('DateTime', module)
  .addDecorator(withKnobs)
  .add(
    'Default',
    () =>
      <DateTime
        dateTime={
          moment(
            parseInt(
              date(
                'Date and time', 
                new Date('2016-03-02 13:57')
              )
            )
          ).toISOString()
        } 
      />
  );

Yes, this can be broken down further to make it more readable, but it feels like the API is putting a lot of burden on the developer compared with the other knobs.

@simon360
Copy link
Contributor Author

(sorry... I keep having thoughts about this)

One potential quickish way to improve this would be adding an optional third argument to the date function for formatting, which receives the timestamp as input, and returns the desired output format. So the above would become:

import React from 'react';
import DateTime from './DateTime';
import { date, withKnobs } from '@storybook/addon-knobs';
import moment from 'moment';

storiesOf('DateTime', module)
  .addDecorator(withKnobs)
  .add(
    'Default',
    () =>
      <DateTime
        dateTime={
          date(
            'Date and time', 
            new Date('2016-03-02 13:57'),
            timestamp => moment(parseInt(timestamp)).toISOString()
          )
        } 
      />
  );

It's not perfect, but it's a start?

@danielduan
Copy link
Member

Would love to get a PR for an update to the docs or a potentially better solution.

@grncdr
Copy link
Contributor

grncdr commented Nov 17, 2017

I also use ISO8601 strings to represent timestamps in my application, and this is what I came up with:

import { date } from '@storybook/addon-knobs'

const defaultDate = new Date('2017-11-17T13:00:00Z')

export function timestamp(name: string, defaultValue?: Date): string {
  const value = date(name, defaultValue || defaultDate)
  const d = new Date(value)
  return d.toISOString()
}

Which I use in my components like so:

import { timestamp } from 'path/to/my-custom-knobs'

storiesOf("Some component")
  .add("timestamp knob", () => (
    <SomeComponent timestamp={timestamp('foo')} />
  ))

It might be nice to extend the knobs addon with completely custom knobs (e.g. with a custom UI as well). Is that possible?

grncdr added a commit to grncdr/storybook that referenced this issue Nov 17, 2017
Hypnosphi added a commit that referenced this issue Nov 17, 2017
Document return type of `date` knob (see #1489)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants