Enable expected Enter/Escape keys on Dashboard rename #1508
Conversation
…yup for canceling edit.
| handleKeyUp(e) { | ||
| const {dashboard: {name}, onCancel} = this.props | ||
| if (e.key === 'Escape') { | ||
| this.setState({name}) |
There was a problem hiding this comment.
Seems like onCancel would already perform a setState({name}), and that this handleKeyUp method would simply call onCancel -- is this possible?
There was a problem hiding this comment.
onCancel in DashboardPage.js does not do that, and it shouldn't. This is the proper place to do that. It has all the props information here.
There was a problem hiding this comment.
What purpose is this.setState serving here then? I just tested the code without it, and it seems to have the same effect.
There was a problem hiding this comment.
Oh! I had to think about that for a sec. onCancel actually unmounts the component, which, upon being mounted again, resets its state to what's specified in the constructor. Fair point, @jaredscheib ! This is why PR review is so important. 👍
| render() { | ||
| const {onSave, onCancel} = this.props | ||
| const {name} = this.state | ||
|
|
||
| return ( | ||
| <div className="page-header full-width"> | ||
| <div className="page-header__container"> | ||
| <div className="page-header__left"> | ||
| <form className="page-header__left" onSubmit={this.handleFormSubmit}> |
There was a problem hiding this comment.
I'm not a fan of using forms as it brings up the HTML5 autocomplete (and tooltips upon validation in the TVM). Either way, we should confirm with @nhaugo and @alexpaxton on the design / usage of HTML5 form validation tooltips + autofill in general in our app (ex. in TVM), as they feel discordant with the visual style and UX for the rest of the app.
There was a problem hiding this comment.
I've disabled autoComplete, it should help with the UX continuity.
|
Glad to see this – good work generally. A couple of notes for changes requested. Also, would request a more descriptive title for the PR, ex. "Enable expected Enter/Escape keys on Dashboard rename" |
cryptoquick
changed the title
|
I've made the changes you requested, @jaredscheib 👍 |
|
😍 LGTM! |
Connect #1420 #1421
The problem
Pressing escape for cancel or enter for submit doesn't do the expected thing when renaming a dashboard.
The Solution
Use a form and onSubmit handler for the title, and add a onKeyUp handler that detects escape and returns the contents of the component to its original state while canceling editing.