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

[TextField] Fix enter behavior with multiline when controlled #6992

Merged
merged 5 commits into from May 29, 2017

Conversation

sajal50
Copy link
Contributor

@sajal50 sajal50 commented May 29, 2017

Fixes #6990

  • 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).

@@ -126,7 +126,9 @@ class EnhancedTextarea extends Component {
}

handleChange = (event) => {
this.syncHeightWithShadow(event.target.value);
if ( !this.props.hasOwnProperty('value') ) {
Copy link
Member

Choose a reason for hiding this comment

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

Would if (!this.props.value)) { be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, if the value is set as a falsy value, for example "" (an empty string), then if (!this.props.value) { wouldn't work.

Copy link
Member

Choose a reason for hiding this comment

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

Should good then!

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module! labels May 29, 2017
@oliviertassinari oliviertassinari changed the title [TextField] new line fix when enter is pressed with value prop [TextField] Fix enter behavior with multiline when controlled May 29, 2017
@oliviertassinari
Copy link
Member

A small unit test would make this PR perfect, otherwise, the logic looks good to me 👍 . We might need to port it back to the next branch as might be affected by the same issue.

@sajal50
Copy link
Contributor Author

sajal50 commented May 29, 2017

I have just started learning the testing frameworks mentioned [here] (https://github.com/callemall/material-ui/blob/master/test/README.md). Hoping to add a unit test soon.

I've also started going through the next branch codebase. Will update there as well soon.

@sajal50
Copy link
Contributor Author

sajal50 commented May 29, 2017

  it('does not change the height of text area', (done) => {
    const wrapper = mountWithContext(
      <TextField
        onChange={() => {
          const newHTML = wrapper.find('textarea').at(1).html();
          assert.strictEqual(newHTML, initialHTML, 'should not change the height of the text area');
          done();
        }}
        value="some value"
        id="unique"
        multiLine={true}
      />
    );

    const initialHTML = wrapper.find('textarea').at(1).html();
    wrapper.find('textarea').at(1).simulate('change', {target: {value: '\n'}});
  });

@oliviertassinari How does this look for a unit test? Since, there is not a good API in enzyme to extract the current style instance, this unit test validates the HTML instance itself.

@oliviertassinari
Copy link
Member

@sajal50 I would use the .find() API to target the right element. Then use .props().style to get the style and finally use an assertion with the expected value. Does that help?

@mbrookes mbrookes added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label May 29, 2017
@sajal50
Copy link
Contributor Author

sajal50 commented May 29, 2017

Thank you for your help, @oliviertassinari. For some reason, .props() method wasn't working on a native HTML element, such as <textarea>, before. It works now. I have pushed the unit test.

@oliviertassinari
Copy link
Member

@sajal50 Thanks, I have removed that test as wasn't properly testing the behavior (it's green on master without the fix). Testing that part is hard without the test infrastructure we have on the next branch, so going to merge without 👍 .

@oliviertassinari oliviertassinari merged commit d958158 into mui:master May 29, 2017
@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels May 29, 2017
@neemah
Copy link

neemah commented Jun 5, 2017

Can't wait release with this one! :)

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 6, 2017

@neemah It's done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants