Janitorial: Refactor site-redirect domain management component #16549
Conversation
aidvu
commented
Jul 25, 2017
aidvu
commented
- ES6ify
- Use Redux actions for tracking
- Remove unnecessary file
- Fix broken input and loading placeholders
- ES6ify - Use Redux actions for tracking - Remove unnecessary file - Fix broken input and loading placeholders
aidvu
added
[Feature Group] Emails & Domains
matticbot
added
the
[Size] XL
|
|
||
| handleChange( event ) { | ||
| handleChange = ( event ) => { | ||
| let location = event.target.value; | ||
|
|
||
| // Removes the protocol part | ||
| location = location.replace( /.*:\/\//, '' ); |
There was a problem hiding this comment.
Seems like a good idea to also use withoutHttp here, like it was done in https://github.com/Automattic/wp-calypso/pull/16193/files#diff-6564d86c047bc86c6f216e59c29aee42R79.
There was a problem hiding this comment.
My only problem is that in theory the redirect could be to any protocol. Gah, maybe we should make stripProtocol/withoutProtocol?
| }; | ||
|
|
||
| state = { | ||
| location: this.props.location.value |
There was a problem hiding this comment.
Wait, location is both in the state and props? 🤦♂️ Maybe we could at least name them differently? The one in state could be something like locationValue.
| let classes = classNames( | ||
| const { location, translate } = this.props; | ||
| const { isUpdating, notice } = location; | ||
| const isFetching = location.isFetching || this.state.location === null; |
There was a problem hiding this comment.
This is an example where the props vs state location gets confusing - my initial reaction was "oh, he did not re-use the destructured location he got from props"... then I facepalmed ;)
|
|
||
| <Main className="domain-management-site-redirect"> | ||
| { | ||
| notice && |
There was a problem hiding this comment.
The notice should probably be inside Main, possibly under Header, so that it's the same width as the rest of the view. Otherwise it looks a bit off:
Quick test with the above suggestion:
| name="destination" | ||
| noWrap | ||
| onChange={ this.handleChange } | ||
| onFocus={ this.handleFocus } | ||
| prefix="http://" | ||
| type="text" | ||
| value={ this.state.location } /> | ||
| value={ this.state.location || '' } /> |
There was a problem hiding this comment.
Maybe this logic should be moved to initializing state above?
There was a problem hiding this comment.
Problem is that then we loose the null. :) Well, in theory, empty state should also be invalid. Will try.
| </FormLabel> | ||
|
|
||
| <FormTextInputWithAffixes | ||
| disabled={ this.props.location.isFetching || this.props.location.isUpdating } | ||
| disabled={ isFetching || isUpdating } |
| onClick={ this.handleClick }> | ||
| { this.translate( 'Update Site Redirect' ) } | ||
| { translate( 'Update Site Redirect' ) } | ||
| </FormButton> | ||
|
|
||
| <FormButton |
There was a problem hiding this comment.
From an UX perspective, it might be good to disable the Cancel button too, when we're updating - otherwise, it might seem to the user that it's the button to cancel the update, when in fact it will just navigate away.
klimeryk
added
[Status] Ready to Merge
and removed
[Status] Needs Review
|
Also: fixes #3148 🎉 |
