Introduce enlargedImagePosition prop #5
Conversation
src/ReactImageMagnify.js
Outdated
|
|
||
| let defaultLensStyle = {}; | ||
|
|
||
| switch (enlargedImagePosition) { |
There was a problem hiding this comment.
What do you think about changing this to something like the following? It seems simpler to me.
let defaultLensStyle = {
backgroundColor: 'rgba(0,0,0,.4)'
};
if (enlargedImagePosition === 'over') {
defaultLensStyle = {};
}
There was a problem hiding this comment.
Yes, you're right, it's simpler.
src/ReactImageMagnify.js
Outdated
| @@ -123,13 +141,15 @@ ReactImageMagnify.propTypes = { | |||
| largeImage: ImageShape, | |||
| lensStyle: PropTypes.object, | |||
| smallImage: ImageShape, | |||
| style: PropTypes.object | |||
| style: PropTypes.object, | |||
| enlargedImagePosition: PropTypes.string | |||
There was a problem hiding this comment.
What do you think about changing the prop validation to React.PropTypes.oneOf(['beside', 'over'])? I'm thinking It helps to document the API and it might give valuable feedback in the scenario where a developer has a typo or uses the wrong word by mistake. If using oneOf breaks the test for prop types (it has correct prop types), then I think I am okay with removing the test, or finding a different (better) way to test prop types. The current test for prop types is not supported by React and is not a standard method in the community. Please let me know your thoughts!
There was a problem hiding this comment.
It's a very good idea, I did not think about that, I'll change that quickly.
src/ReactImageMagnifyTouch.js
Outdated
| @@ -91,11 +93,13 @@ ReactImageMagnifyTouch.propTypes = { | |||
| pressDuration: PropTypes.number, | |||
| pressMoveThreshold: PropTypes.number, | |||
| smallImage: ImageShape, | |||
| style: PropTypes.object | |||
| style: PropTypes.object, | |||
| enlargedImagePosition: PropTypes.string | |||
There was a problem hiding this comment.
Please see my comment above.
|
@damien916 I think this is a great feature! Thank you very much for creating it! I left a couple comments on your PR. Please let me know what you think about them. ~Ethan |
|
@ethanselzer I just add two commits to the PR. I don't finded solution to test oneOf PropType with enzyme, so I commented them. Thank you again. Damien |
|
@damien916 - Thanks for making the adjustments I suggested! I really appreciate it. The npm package has been updated and a new release has been created on GitHub. You are awesome! ✨💫 |
|
@ethanselzer Thank you so much 👍 |
Hello @ethanselzer
I just add optional prop enlargedImagePosition to choose position of enlarged image :
Have a nice day :)