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

[DropDownMenu] [SelectField] Add defaultText prop shown when there's no item with the corresponding value #4902

Closed
wants to merge 1 commit into from

Conversation

zalmoxisus
Copy link

@zalmoxisus zalmoxisus commented Aug 4, 2016

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

Redo of #4822.

Normally (at least accordingly to HTML specs), a select box shouldn't be empty, unless you there's an empty option, even if the value doesn't correspond to any item. So, you can specify a string in defaultText prop to be shown in this case.

@GuillaumeCisco
Copy link
Contributor

Does it work with hintText?
So we should only specify defaultDisplayValue={} and hintText={'my placeholder'}?

@zalmoxisus
Copy link
Author

When defaultDisplayValue isn't specified, it will work as in the earlier versions. But when both are specified, they will still overlap each other. Not sure which one we should prioritize , and which to ignore in this case.

@zalmoxisus
Copy link
Author

zalmoxisus commented Aug 4, 2016

Added the prop also to SelectField, so it will be used only when no hintText and floatingLabelText specified, fixing all the issues.

@muibot muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Aug 4, 2016
@zalmoxisus zalmoxisus changed the title [DropDownMenu] Add defaultDisplayValue prop shown when there's no item with the corresponding value [DropDownMenu] [SelectField] Add defaultText prop shown when there's no item with the corresponding value Aug 4, 2016
@@ -180,6 +185,7 @@ class SelectField extends Component {
<TextField
{...other}
style={style}
defaultText={!hintText && !floatingLabelText ? defaultText : ''}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe !(hintText || floatingLabelText) ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right 👍

@muibot muibot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 4, 2016
@@ -180,6 +185,7 @@ class SelectField extends Component {
<TextField
{...other}
style={style}
defaultText={hintText || floatingLabelText ? '' : defaultText}
Copy link
Member

@oliviertassinari oliviertassinari Aug 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case for this property?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, for all the cases I have, there's a default action when nothing is selected, but I don't want to keep the select box empty. A simple solution would be to specify the text from the first option here as well, as other people still want this select empty for their cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm asking because defaultText doesn't seems to have any impact on the TextField component. And React is throwing a warning about leaking property.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snap, you're right. It was intended to be passed for DropDownMenu.

Does this parameter still make sense or we'll use the first option's value?

Copy link
Member

@oliviertassinari oliviertassinari Aug 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll use the first option's value?

I think that it would be better without this property. It isn't going to help migrating from a <select /> to a <SelectField />, nor the other way around, right?

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 4, 2016

@zalmoxisus As far as I have investigated the issue, we need the <TextField /> to know about the state of the <DropDownMenu />.

But the TextField is a parent of the DropDownMenu. I think that it should be the other way around. Having this order makes use do non-intuitive stuff like:

  • Accessing the children value with:
const propsLeaf = children ? children.props : this.props;
  • Expose this imperative method:
  /**
   * This method is deprecated but still here because the TextField
   * need it in order to work. TODO: That will be addressed later.
   */
  getInputNode() {
    const root = this.refs.root;

    root.focus = () => {
      if (!this.props.disabled) {
        this.setState({
          open: !this.state.open,
          anchorEl: this.refs.root,
        });
      }
    };

    return root;
  }

@oliviertassinari
Copy link
Member

I'm closing this PR as quite old now and inactive. Thanks helping us moving forward.
I have gathered some thought around how we could improve thing here #4891 (comment).
That may require quite some work, so doing on the next branch might be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants