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

Pickers should have isRequired property #6636

Closed
tsekityam opened this issue Oct 10, 2018 · 9 comments
Closed

Pickers should have isRequired property #6636

tsekityam opened this issue Oct 10, 2018 · 9 comments

Comments

@tsekityam
Copy link
Contributor

tsekityam commented Oct 10, 2018

Describe the feature that you would like added
Add ability to mark any picker as a required field

What component or utility would this be added to
Pickers

Have you discussed this feature with our team, and if so, who
No.

Additional context/screenshots

@micahgodbolt
Copy link
Member

@joschect ever considered adding 'required' to pickers?

@tsekityam
Copy link
Contributor Author

@micahgodbolt @joschect DatePicker already has isRequired property, I think we should promote this to other pickers as well.

BTW, because the required property in DatePicker is named isRequired, so I propose a new property named isRequired for the consistency.

@tsekityam tsekityam changed the title Poeple Picker should have isRequired property Pickers should have isRequired property Oct 10, 2018
@micahgodbolt
Copy link
Member

@tsekityam yes to consistency, but the opposite direction. We've been transitioning any 'isRequired' or 'isDisabled' to their html attribute name for consistency. So we'll probably deprecate isRequired on date picker and make 'required' the value for all of them.

@joschect
Copy link
Contributor

Would we just want pickers required to act just like a textfield required? I do think it makes sense to add it.

@micahgodbolt
Copy link
Member

Seeing lots of issues that might be resolved by separating the control from the control inside of a form. If we add required, do we also need to add error messages? It'd be nice to abstract error messaging and make sure that each control supports a required visual variation

@micahgodbolt
Copy link
Member

might be work bundled into #5582

@j-te
Copy link

j-te commented Aug 16, 2019

Seeing lots of issues that might be resolved by separating the control from the control inside of a form. If we add required, do we also need to add error messages? It'd be nice to abstract error messaging and make sure that each control supports a required visual variation

I think Wrapping a form component could work well IE new component <Validate> looks at its children:

<Validate
  isError={isError}
  errorType={errorTypeEnum}
  errorMessage={props => someUtilFunc(props)}
>
    <TextField
      label="Title" 
      value={value}
      styles={props => ({
            fieldGroup: props.isError ? { background: "red" }
      })}
    />

    <DatePicker
      {...datePickerProps}
    />
</Validate>

This way we can have one component that can check a group of inputs, helps reduce re-rendering/unnecessary duplication.

isError & errorType props could be passed down to it's children, so that further styling could be applied (example, colour shadow highlight on error type).

I find that base input components that naturally change layout dimensions is bad practice and can cause unnecessary re-renders so keeping it out of the base components would be my preference. It's the reason I've avoided the description prop.

@gioce90
Copy link

gioce90 commented Sep 17, 2019

When we can have the required propertyfor the BasePicker? Iit's really limiting not having it.

@khmakoto
Copy link
Member

Closing this in favor of #3620.

@microsoft microsoft locked as resolved and limited conversation to collaborators Feb 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants