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

[DatePicker] How do you get the name/event of the control? Should we handle "event"? #6538

Closed
oshalygin opened this issue Apr 7, 2017 · 8 comments
Labels
component: date picker This is the name of the generic UI component, not the React module! v0.x

Comments

@oshalygin
Copy link

oshalygin commented Apr 7, 2017

Problem description

I looked through the docs and it looks like we're missing the event property that is null, which is used to properly set the values of the controlled component(such as TextField).

For instance to do the following:
property = event.target.name; is not possible with a DatePicker because the event is null.

The date property is properly hydrated but it makes things a bit wonky not being able to access the event itself, specifically the name of the component.

Maybe we just need to add a name property?

What do you guys think?

EDIT: Looks like the biggest issue is when you want two DatePicker controls within the same root component.

Link to minimal working code that reproduces the issue

     <DatePicker
          fullWidth
          style={{ margin: '-1rem auto' }}
          textFieldStyle={{ fontSize: fonts.md, marginTop: '0.25rem' }}
          name={propertyName}
          value={propertyValue}
          onChange={this.onDateChange}
        />

  onDateChange(event, date) {
    console.log(event); //null
    console.log('date', date); //whatever the date the user picked
  }

Versions

  • Material-UI: 0.17.1
  • React: 15.4.2
  • Browser: Latest Chrome
@oshalygin
Copy link
Author

oshalygin commented Apr 7, 2017

I ended up wrapping the Material UI component and I'm going to move forward. Works kind of nice based on what I need. Here's the code snippet.

import React, { PropTypes } from 'react';
import BaseDatePicker from 'material-ui/DatePicker';

class DatePicker extends React.Component {
  
  constructor() {
    super();
    this.state = {};

    this.onChange = this.onChange.bind(this);
    this.convertToMillis = this.convertToMillis.bind(this);
    this.convertToDateString = this.convertToDateString.bind(this);
  }

  convertToMillis(dateString) {
    return dateString.valueOf();
  }

  // Note that the default value for a MaterialUI DatePicker is {}
  // NOT an empty string or any other value type
  convertToDateString(dateMillis) {
    if (dateMillis !== null && typeof yourVariable === 'object') {
      return dateMillis;
    }
    return new Date(dateMillis);
  }

  // Note that event is always null, but it is the first arg.
  // The reason it is null and it exists is because MUI wanted to maintain consistency
  // with the Textfield control.
  // http://www.material-ui.com/#/components/date-picker
  onChange(event, date) { //eslint-disable-line no-unused-vars
    const { onDataChange, name } = this.props;
    const value = this.convertToMillis(date);
    onDataChange(name, value);
  }

  render() {
    const {
      name,
      valueMillis,
      fullWidth,
      style,
      textFieldStyle
    } = this.props;

    const value = this.convertToDateString(valueMillis);
    
    return (
      <BaseDatePicker
        onChange={this.onChange}
        name={name} // This is required as Material-UI needs a key index on TextField
        value={value}
        fullWidth={fullWidth}
        style={style}
        textFieldStyle={textFieldStyle}/>
    );
  }
}

DatePicker.propTypes = {
  name: PropTypes.string.isRequired,
  onDataChange: PropTypes.func.isRequired,
  valueMillis: PropTypes.any.isRequired,
  fullWidth: PropTypes.bool,
  style: PropTypes.object,
  textFieldStyle: PropTypes.object
};

export default DatePicker;

But I would be interested to see what you guys are thinking long term for DatePicker because I'd love to help more.

@mbrookes
Copy link
Member

mbrookes commented Apr 8, 2017

@oshalygin DatePicker needs a rewrite as part of the migration to the new styling system on the next branch. Standardising callbacks is part of that effort, but no-one has tackled the pickers yet.

How much help were you thinking? 😜

@oshalygin
Copy link
Author

Yeah I'd love to help as much as I can.

I don't think it would be too bad based on what I did to wrap the current DatePicker but I totally get that it will need to leverage the new styling effort.

I'll start tracking the next branch and digging deeper over the next week :)

@mbrookes
Copy link
Member

mbrookes commented Apr 8, 2017

@oshalygin Fantastic! There's a lot going on inside those components, so it's more than just updating to jss (although that's a big part of it). We're working towards:

  • Separating presentation and state
  • Composable components (where appropriate)
  • Standardising the API (where possible)
  • Squashing bugs
  • Tests! 🎉
  • Accessibility (ARIA labels, full keyboard support)
  • Improving the documentation

@oliviertassinari will be a good person to discuss possible approaches with. We have this placeholder issue (#4787).

@oliviertassinari oliviertassinari added the component: date picker This is the name of the generic UI component, not the React module! label Apr 8, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 8, 2017

@oshalygin Yeah, the DatePicker, and TimePicker definitely need to be reworked! You can find all the relevant issues using the tag filter. We need someone to take the ownership of the migration 😄
I want to add some other stuff that are disturbing me with the two components:

  • UX
    • Not desktop optimized, for instance:
      • users might want to use their keyboard
      • the mouse travels too much distance
    • Not mobile optimized, for instance:
      • phone's OS have already have pretty good date picker and time picker, why reimplementing the wheel with a custom component?
  • The API
    • We miss quite some customization power, we could break the component into multiple indepentant one.

@oshalygin
Copy link
Author

Awesome thank you gents, @oliviertassinari @mbrookes . Going to get crackin!

Closing this issue out and will track with the filter.

@TuhinAtha
Copy link

@oshalygin I am still facing the same issue. Using v0.20.0.

@dzimukairu
Copy link

just a heads-up, still facing this issue.

@mui mui locked as resolved and limited conversation to collaborators Feb 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: date picker This is the name of the generic UI component, not the React module! v0.x
Projects
None yet
Development

No branches or pull requests

5 participants