Migrate view_image.jsx to be follow the guidline #204
Conversation
components/view_image.jsx
Outdated
| @@ -114,7 +141,7 @@ export default class ViewImageModal extends React.Component { | |||
| showImage(id) { | |||
| this.setState({imageIndex: id}); | |||
There was a problem hiding this comment.
Could you remove function binding above (e.g. this.showImage = this.showImage.bind(this);showImage = (id) => { ... } ?
| @@ -1,31 +1,58 @@ | |||
| // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | |||
| // See License.txt for license information. | |||
|
|
|||
| import $ from 'jquery'; | |||
|
|
|||
components/view_image.jsx
Outdated
| fileInfos: PropTypes.arrayOf(PropTypes.object).isRequired, | ||
| startIndex: PropTypes.number | ||
| }; | ||
|
|
||
| function LoadingImagePreview({progress, loading}) { |
There was a problem hiding this comment.
I suggest the following to be on a separate file as a separate pure component with corresponding component tests:
- LoadingImagePreview to loading_image_preview.jsx
- ImagePreview to image_preview.jsx
There was a problem hiding this comment.
@saturninoabril Separating component files is a good idea. They don't have a state, so can I leave it in a stateless-functional style, not a pure component?
There was a problem hiding this comment.
Yes, it'd be fine as stateless functional component.
tests/components/view_image.test.jsx
Outdated
|
|
||
| expect(wrapper).toMatchSnapshot(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test for ViewImageModal is a bit low. You may find related samples at tests/components. I'd suggest to at least cover the following:
- handleNext
- handlePrev
- handleKeyPress
- onModalShown
- onModalHidden
- componentWillReceiveProps
- showImage
- loadImage
- handleImageLoaded
- handleImageProgress
- handleGetPublicLink
- onMouseEnterImage
- onMouseLeaveImage
|
Thank you @saturninoabril @jwilander for your review! I will take a look for it soon 😄 |
8936138
to
692c1ec
Compare
components/view_image.jsx
Outdated
| @@ -198,7 +211,6 @@ export default class ViewImageModal extends React.Component { | |||
| content = ( | |||
| <ImagePreview | |||
| fileInfo={fileInfo} | |||
| fileUrl={fileUrl} | |||
There was a problem hiding this comment.
Hey @saturninoabril @jwilander.
I just moved this line into child component which I separated. Should I do this same for all other preview components, or restore?
There was a problem hiding this comment.
No need. If necessary, separate ticket will be created for it.
|
@cometkim You have failing test, at least in |
|
I don't know why modal component of react-bootstrap makes errors on enzyme rendering... |
|
@cometkim I'll take a look at this today. |
…op of the component
68ffc28
to
d4b02c2
Compare
|
@cometkim I have pushed one on this commit, adding/fixing tests. |
|
Amazing!! Thank you @saturninoabril (BTW, I want to see how tests work, but the github is too slow to my internet now) |
|
Nice work on this @cometkim! I'll merge after the 4.4 release branch gets cut |
* Add reducers, actions and client functions for user access tokens * Add utils functions for new roles * Update role checking utility to prevent false positives * Add admin reducers for user access tokens * Add action to clear user access tokens * Add tracking to access token client functions * Fix ESLint error * Updates per feedback
* Add reducers, actions and client functions for user access tokens * Add utils functions for new roles * Update role checking utility to prevent false positives * Add admin reducers for user access tokens * Add action to clear user access tokens * Add tracking to access token client functions * Fix ESLint error * Updates per feedback
Summary
Migrate view_image.jsx to be follow the guidline.
Ticket Link
mattermost/mattermost#7684
Checklist