feat(react/components/media-cropper): added the media-cropper compone… #182
Conversation
|
Is this still being worked on @thomascking? If so, there is no need to assign reviewers until you are done. Are you just looking for a conversation on these components? |
It is still being worked on, but I was looking for some input on both the code and some concepts, namely the ones in the TODOS and Potential Improvements section of the README. |
|
Gotcha Thanks. Is this an entire react app? Or is this just the component? |
There is a little demo app that can be run, but it is really just the component. |
| "crop" | ||
| ], | ||
| "author": "Thomas King", | ||
| "license": "ISC", |
There was a problem hiding this comment.
What is our license for this repo?
There was a problem hiding this comment.
I believe MIT @michaelachrisco
| componentDidUpdate(props, state) { | ||
| if (this.state.dragging && !state.dragging) { | ||
| document.addEventListener('mousemove', this.mouseMove); | ||
| document.addEventListener('mouseup', this.mouseUp); | ||
| document.addEventListener('mouseleave', this.mouseUp); | ||
| } else if (!this.state.dragging && state.dragging) { | ||
| document.removeEventListener('mousemove', this.mouseMove); | ||
| document.removeEventListener('mouseup', this.mouseUp); | ||
| document.removeEventListener('mouseleave', this.mouseUp); | ||
| } |
There was a problem hiding this comment.
How to do feel about moving these to useEvent or useSelector react components for detecting component updates?
There was a problem hiding this comment.
Sorry, not really experienced with React, what are those?
There was a problem hiding this comment.
No problem, Im sorta new too!
Those are all part of https://react-redux.js.org
Specifically Hooks: https://react-redux.js.org/api/hooks
I think they are neat.
There was a problem hiding this comment.
oh, gotcha. This component will not be using redux as its state should be self-contained. Is there a particular reason you feel that would be useful here?
There was a problem hiding this comment.
A good reason for using hooks, specifically (those are not redux-specific, but rather React core) is that they are the way that modern React handles local state.
| } | ||
|
|
||
| mouseWheel(e) { | ||
| const scale = Math.pow(1.0015, -e.deltaY); |
There was a problem hiding this comment.
What is this indicating?
There was a problem hiding this comment.
this is used for zooming, first we need to know how much we are adjusting the zoom (that is the scale). then we can calculate how to adjust the translation based on the change in zoom. This allows us to "zoom around a point."
| media.muted = true; | ||
| media.src = URL.createObjectURL(file); | ||
| media.currentTime = 10; | ||
| media.play(); |
There was a problem hiding this comment.
Will media.play automatically start playing the video?
There was a problem hiding this comment.
yes, it will start loading the video and play it as soon as it is ready; it is used here to force the video to load and render the first the frame (in this case the one at 10 seconds that I set for testing).
There was a problem hiding this comment.
Can we make this configurable?
I'm not sure if we should have any complete apps within the s/p repo? What does everyone else think? I'm concerned about maintainability of the packages over the lifespan of the repo. |
…ogress indicator for video renderer
Agreed. I think "proof of concept" style apps should live in their own repos, while reusable components or snippets can live in this repo. We will have a more defined and standardized practice for how we develop and maintain components once we start developing reusable components for the boilerplates. |
There was a problem hiding this comment.
This is GREAT work @thomascking! I do agree with @michaelachrisco & @coreyshuman that this should be split out into just the component pieces rather than an entire app if it's going to go into the reusable components folder.
I was also thinking that we should add strict typing & at least an I/O Jest test before we merge it in. We are trying our best as an org to add testing & strict types to all our reusable stuff.
Beyond that, if we could provide a Functional component-based version of this using hooks, that would be awesome, but not necessary.
Overall, this is really great solid work, thanks for contributing! Let me know if you want any help with any of those asks. I'll be refactoring some of the other reusable components with tests/strict typing as well.
| @@ -0,0 +1,59 @@ | |||
| # Magtek Card Reader Middleware | |||
There was a problem hiding this comment.
We need to change this header, don't we?
| this.state = { | ||
| zoom: 1, | ||
| transX: 0, | ||
| transY: 0, | ||
| media: null, | ||
| isVideo: false, | ||
| dragging: false, | ||
| mouseStart: { | ||
| x: 0, |
There was a problem hiding this comment.
Nothing wrong with using Class-based components, but the more modern React projects we are working on currently use the newer hooks to manage state within a component.
There was a problem hiding this comment.
I'll look into converting this, I think my Java background leads me to defaulting to class-based solutions.
| }; | ||
|
|
||
| reader.readAsDataURL(file); | ||
| } else if (file.type.startsWith('video/')) { |
There was a problem hiding this comment.
Are we sure that file.type would always start with video/ or was this specific to this project? If we aren't enforcing that structure, we should be somehow.
There was a problem hiding this comment.
All images should be type "image/" and videos should be "video/" I assumed. do you know of some video files that would have a different type?
| } | ||
| } | ||
| } else { | ||
| console.log(file.type); |
There was a problem hiding this comment.
I would remove all logs before this gets merged, or add TODOs to remove them in production.
| <div | ||
| onClick={this.click} | ||
| onMouseDown={this.mouseDown} | ||
| onWheel={this.mouseWheel} | ||
| > | ||
| {Renderer ? <Renderer | ||
| ref="media" | ||
| {...this.state} | ||
| width={this.props.width} | ||
| height={this.props.height} | ||
| /> : <div>Click to add.</div>} | ||
| <input | ||
| type="file" | ||
| id="file" | ||
| ref="fileUploader" | ||
| accept="image/*,video/*" | ||
| style={{ display: "none" }} | ||
| onChange={this.loadFile} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Very clean!! This is super cool!
| componentDidMount() { | ||
| this.updateContext(); | ||
| } | ||
|
|
||
| componentDidUpdate(props, state) { | ||
| // if we are switching to rendering | ||
| if (!state.rendering && this.state.rendering) { | ||
| console.log('freezing state'); | ||
| // freeze the current state | ||
| this.setState({ ...this.state, ...this.props }); | ||
| } | ||
| this.renderCanvas(); | ||
| } |
There was a problem hiding this comment.
Related to the earlier comments about hooks, the useEffect hook is super cool for replacing these different event hooks.
| return (<canvas | ||
| ref="canvas" | ||
| width={this.props.width} | ||
| height={this.props.height} | ||
| ></canvas>); |
There was a problem hiding this comment.
Excellent use of canvas. NIce!
| @@ -0,0 +1,32 @@ | |||
| .video-renderer { | |||
There was a problem hiding this comment.
Would there be any value in making these styles configurable with props?
There was a problem hiding this comment.
I think that the colors for sure being configurable makes sense.
| } | ||
|
|
||
| componentDidMount() { | ||
| super.componentDidMount(); |
| return ( | ||
| <div> | ||
| <div className="round"> | ||
| <MediaCropper ref="cropper" height={270} width={300} /> |
There was a problem hiding this comment.
Do we want the height and width values to be static?
|
@thomascking Are you still working on this PR? If so, do you mind moving off |
|
@thomascking Are you still working on this PR? |
|
@thomascking Do you mind if I close this one out? Or are you still working on this PR? |
|
Closing as I didnt get a response back. |
…nt for react
Changes
Approach
Created a custom component that handles inline image cropping within a container of any shape so a user can easily see that the final product will look like.
Learning
Images and videos are rectangular by nature. Because of this, most image cropping tools only allow you to crop an image to a rectangle. Some have also added an ellipse option as well, but what happens when you have an oddly shaped media display?
Furthermore, most croppers can only handle static images. The goal here was to add support for both animated gif as well as video files. Instead of directly cropping videos, however, I believe it is best to just output some data that can be sent to a service that will actually handle the video manipulation (likely with ffmpeg).
This is only a draft
I will work to iron out some of the details, but reviews and suggestions are very welcome.
Closes #181