This repository has been archived by the owner on Mar 13, 2024. It is now read-only.
[MM-12068] Add ability to remove custom branding image #3207
Merged
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
cb29837
[MM-12068] Added the ability to remove custom branding image
hahmadia aba04b1
[MM-12068] Fixed lint error
hahmadia 37a0db0
[MM-12068] Update en.json
hahmadia 7d437f2
[MM-12068] Added styling that was accidently removed
hahmadia eaee0ae
[MM-12068] Addressed PR comments
hahmadia ed96ac7
[MM-12068] Fixed test case
hahmadia e454870
[MM-12068] Addressed PR comments
hahmadia 77c493b
[MM-12068] Fixed Asyncrhonous behaviour
hahmadia a257440
[MM-12068] Add function comments
hahmadia e3a4de4
[MM-12068] Removed unnecessary code
hahmadia d63787a
[MM-12068] Fixed small code discrepancy
hahmadia d1764b6
[MM-12068] Generalised code
hahmadia 515a66b
[MM-12068] Address comments regarding incorrect ref handling and made…
hahmadia c297a39
[MM-12068] Lint error fixes
hahmadia 63d2203
[MM-12068] Addressed PR and used function pass instead of refs
hahmadia fa1bb09
[MM-12068] return error as object and refactor code
hahmadia b704630
[MM-12068] Add unregister save action function
hahmadia 00999cf
[MM-12068] Lint and snapshot fixes
hahmadia ef14ae1
Merge branch 'master' into MM-12068
hahmadia c76e151
Merge branch 'master' into MM-12068
hahmadia 1b155f1
add unit test
saturninoabril File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,14 +5,13 @@ import $ from 'jquery'; | |
import PropTypes from 'prop-types'; | ||
import React from 'react'; | ||
import {FormattedHTMLMessage, FormattedMessage} from 'react-intl'; | ||
import {OverlayTrigger, Tooltip} from 'react-bootstrap'; | ||
import {Client4} from 'mattermost-redux/client'; | ||
|
||
import {uploadBrandImage} from 'actions/admin_actions.jsx'; | ||
import {UploadStatuses} from 'utils/constants.jsx'; | ||
import {uploadBrandImage, deleteBrandImage} from 'actions/admin_actions.jsx'; | ||
import {Constants} from 'utils/constants.jsx'; | ||
import FormError from 'components/form_error.jsx'; | ||
|
||
import UploadButton from './upload_button.jsx'; | ||
|
||
const HTTP_STATUS_OK = 200; | ||
|
||
export default class BrandImageSetting extends React.PureComponent { | ||
|
@@ -22,20 +21,35 @@ export default class BrandImageSetting extends React.PureComponent { | |
* Set to disable the setting | ||
*/ | ||
disabled: PropTypes.bool.isRequired, | ||
|
||
/* | ||
* Set the save needed in the admin schema settings to trigger the save button to turn on | ||
*/ | ||
setSaveNeeded: PropTypes.func.isRequired, | ||
|
||
/* | ||
* Registers the function suppose to be run when the save button is pressed | ||
*/ | ||
registerSaveAction: PropTypes.func.isRequired, | ||
|
||
/* | ||
* Unregisters the function on unmount of the component suppose to be run when the save button is pressed | ||
*/ | ||
unRegisterSaveAction: PropTypes.func.isRequired, | ||
} | ||
|
||
constructor(props) { | ||
super(props); | ||
|
||
this.handleImageChange = this.handleImageChange.bind(this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a stopper, but we normally prefer to use "arrow functions" to avoid the need of binding this, for example if you define the handleImageChange = () => {
...
} instead of handleImageChange() {
...
} You wont need the binding. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to file a help wanted ticket to clean these up across the code base, so it doesn't need to be done in this PR. We've switched most of our new code to use arrow functions, but there's quite a few older components where we're still doing this. |
||
this.handleImageSubmit = this.handleImageSubmit.bind(this); | ||
this.handleDeleteButtonPressed = this.handleDeleteButtonPressed.bind(this); | ||
|
||
this.state = { | ||
deleteBrandImage: false, | ||
brandImage: null, | ||
brandImageExists: false, | ||
brandImageTimestamp: Date.now(), | ||
error: '', | ||
status: UploadStatuses.DEFAULT, | ||
}; | ||
} | ||
|
||
|
@@ -51,6 +65,14 @@ export default class BrandImageSetting extends React.PureComponent { | |
); | ||
} | ||
|
||
componentDidMount() { | ||
this.props.registerSaveAction(this.handleSave); | ||
} | ||
|
||
componentWillUnmount() { | ||
this.props.unRegisterSaveAction(this.handleSave); | ||
} | ||
|
||
componentDidUpdate() { | ||
if (this.refs.image) { | ||
const reader = new FileReader(); | ||
|
@@ -66,56 +88,64 @@ export default class BrandImageSetting extends React.PureComponent { | |
|
||
handleImageChange() { | ||
const element = $(this.refs.fileInput); | ||
|
||
if (element.prop('files').length > 0) { | ||
this.props.setSaveNeeded(); | ||
this.setState({ | ||
brandImage: element.prop('files')[0], | ||
status: UploadStatuses.DEFAULT, | ||
deleteBrandImage: false, | ||
}); | ||
} | ||
} | ||
|
||
handleImageSubmit(e) { | ||
e.preventDefault(); | ||
|
||
if (!this.state.brandImage) { | ||
return; | ||
} | ||
|
||
if (this.state.status === UploadStatuses.LOADING) { | ||
return; | ||
} | ||
handleDeleteButtonPressed() { | ||
this.setState({deleteBrandImage: true, brandImage: null, brandImageExists: false}); | ||
this.props.setSaveNeeded(); | ||
} | ||
|
||
handleSave = async () => { | ||
this.setState({ | ||
error: '', | ||
status: UploadStatuses.LOADING, | ||
}); | ||
|
||
uploadBrandImage( | ||
this.state.brandImage, | ||
() => { | ||
this.setState({ | ||
brandImageExists: true, | ||
brandImage: null, | ||
brandImageTimestamp: Date.now(), | ||
status: UploadStatuses.COMPLETE, | ||
}); | ||
}, | ||
(err) => { | ||
this.setState({ | ||
error: err.message, | ||
status: UploadStatuses.DEFAULT, | ||
}); | ||
} | ||
); | ||
let error; | ||
if (this.state.deleteBrandImage) { | ||
await deleteBrandImage( | ||
() => { | ||
this.setState({ | ||
deleteBrandImage: false, | ||
brandImageExists: false, | ||
brandImage: null, | ||
}); | ||
}, | ||
(err) => { | ||
error = err; | ||
this.setState({ | ||
error: err.message, | ||
}); | ||
} | ||
); | ||
} else if (this.state.brandImage) { | ||
await uploadBrandImage( | ||
this.state.brandImage, | ||
() => { | ||
this.setState({ | ||
brandImageExists: true, | ||
brandImage: null, | ||
brandImageTimestamp: Date.now(), | ||
}); | ||
}, | ||
(err) => { | ||
error = err; | ||
this.setState({ | ||
error: err.message, | ||
}); | ||
} | ||
); | ||
} | ||
return {error}; | ||
} | ||
|
||
render() { | ||
let btnPrimaryClass = 'btn'; | ||
if (this.state.brandImage) { | ||
btnPrimaryClass += ' btn-primary'; | ||
} | ||
|
||
let letbtnDefaultClass = 'btn'; | ||
if (!this.props.disabled) { | ||
letbtnDefaultClass += ' btn-default'; | ||
|
@@ -124,24 +154,53 @@ export default class BrandImageSetting extends React.PureComponent { | |
let img = null; | ||
if (this.state.brandImage) { | ||
img = ( | ||
<img | ||
ref='image' | ||
className='brand-img' | ||
alt='brand image' | ||
src='' | ||
/> | ||
<div className='remove-image__img margin-bottom x3'> | ||
<img | ||
ref='image' | ||
alt='brand image' | ||
src='' | ||
/> | ||
</div> | ||
); | ||
} else if (this.state.brandImageExists) { | ||
let overlay; | ||
if (!this.props.disabled) { | ||
overlay = ( | ||
<OverlayTrigger | ||
delayShow={Constants.OVERLAY_TIME_DELAY} | ||
placement='right' | ||
overlay={( | ||
<Tooltip id='removeIcon'> | ||
<div aria-hidden={true}> | ||
<FormattedMessage | ||
id='admin.team.removeBrandImage' | ||
defaultMessage='Remove brand image' | ||
/> | ||
</div> | ||
</Tooltip> | ||
)} | ||
> | ||
<button | ||
className='remove-image__btn' | ||
onClick={this.handleDeleteButtonPressed} | ||
> | ||
<span aria-hidden={true}>{'×'}</span> | ||
</button> | ||
</OverlayTrigger> | ||
); | ||
} | ||
img = ( | ||
<img | ||
className='brand-img' | ||
alt='brand image' | ||
src={Client4.getBrandImageUrl(this.state.brandImageTimestamp)} | ||
/> | ||
<div className='remove-image__img margin-bottom x3'> | ||
<img | ||
alt='brand image' | ||
src={Client4.getBrandImageUrl(this.state.brandImageTimestamp)} | ||
/> | ||
{overlay} | ||
</div> | ||
); | ||
} else { | ||
img = ( | ||
<p> | ||
<p className='margin-top'> | ||
<FormattedMessage | ||
id='admin.team.noBrandImage' | ||
defaultMessage='No brand image uploaded' | ||
|
@@ -159,18 +218,20 @@ export default class BrandImageSetting extends React.PureComponent { | |
/> | ||
</label> | ||
<div className='col-sm-8'> | ||
{img} | ||
<div className='remove-image'> | ||
{img} | ||
</div> | ||
</div> | ||
<div className='col-sm-4'/> | ||
<div className='col-sm-8'> | ||
<div className='file__upload'> | ||
<div className='file__upload margin-top x3'> | ||
<button | ||
className={letbtnDefaultClass} | ||
disabled={this.props.disabled} | ||
> | ||
<FormattedMessage | ||
id='admin.team.chooseImage' | ||
defaultMessage='Choose New Image' | ||
defaultMessage='Select Image' | ||
/> | ||
</button> | ||
<input | ||
|
@@ -181,12 +242,6 @@ export default class BrandImageSetting extends React.PureComponent { | |
onChange={this.handleImageChange} | ||
/> | ||
</div> | ||
<UploadButton | ||
primaryClass={btnPrimaryClass} | ||
status={this.state.status} | ||
disabled={this.props.disabled || !this.state.brandImage} | ||
onClick={this.handleImageSubmit} | ||
/> | ||
hahmadia marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<br/> | ||
<FormError error={this.state.error}/> | ||
<p className='help-text no-margin'> | ||
|
42 changes: 42 additions & 0 deletions
42
components/admin_console/brand_image_setting/brand_image_setting.test.jsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | ||
// See LICENSE.txt for license information. | ||
|
||
import React from 'react'; | ||
import {shallow} from 'enzyme'; | ||
|
||
import {uploadBrandImage, deleteBrandImage} from 'actions/admin_actions.jsx'; | ||
|
||
import BrandImageSetting from './brand_image_setting.jsx'; | ||
|
||
jest.mock('actions/admin_actions.jsx', () => ({ | ||
...jest.requireActual('actions/admin_actions.jsx'), | ||
uploadBrandImage: jest.fn(), | ||
deleteBrandImage: jest.fn(), | ||
})); | ||
|
||
describe('components/admin_console/brand_image_setting', () => { | ||
const baseProps = { | ||
disabled: false, | ||
setSaveNeeded: jest.fn(), | ||
registerSaveAction: jest.fn(), | ||
unRegisterSaveAction: jest.fn(), | ||
}; | ||
|
||
test('should have called deleteBrandImage or uploadBrandImage on save depending on component state', () => { | ||
const wrapper = shallow( | ||
<BrandImageSetting {...baseProps}/> | ||
); | ||
|
||
const instance = wrapper.instance(); | ||
|
||
wrapper.setState({deleteBrandImage: false, brandImage: 'brand_image_file'}); | ||
instance.handleSave(); | ||
expect(deleteBrandImage).toHaveBeenCalledTimes(0); | ||
expect(uploadBrandImage).toHaveBeenCalledTimes(1); | ||
|
||
wrapper.setState({deleteBrandImage: true, brandImage: null}); | ||
instance.handleSave(); | ||
expect(deleteBrandImage).toHaveBeenCalledTimes(1); | ||
expect(uploadBrandImage).toHaveBeenCalledTimes(1); | ||
}); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're actually moving away from this pattern into a more redux-friendly format which is dispatching an action directly. That can be done by creating
components/admin_console/brand_image_setting/index.js
and from there passuploadBrandImage
anddeleteBrandImage
as actions intobrand_image_setting.jsx
. That would be not necessary for this PR but good to have.Let me know what's your plan. That could also be done on separate PR if you're interested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that can be done separately. I was going to ask for the newly-added action to be changed to the new format, but I didn't because the existing action would also need to be changed in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed through private messages, It was decided that this will be done in as a separate ticket and separate pull request.