feat: planter selfie rotation #142

tranquanghuy0801 · 2021-08-04T13:23:20Z

Resolves #137

nmcharlton

It's a good start, but a few things don't quite work as they should.
We also need to apply the rotation on Planter cards and in the Planter Detail dialog.

nmcharlton · 2021-08-08T10:27:14Z

src/components/ImageScroller.js

+      onSelectChange('imageRotation', 0);
+    } else {
+      setRotation(rotation + 90);
+      onSelectChange('imageRotation', rotation + 90);


You could reduce duplication here by using a temporary variable:

let newRotation = rotation + 90; if (newRotation === 360) { newRotation = 0; } setRotation(newRotation); onSelectChange('imageRotation, newRotation);

nmcharlton · 2021-08-08T10:40:11Z

src/components/ImageScroller.js

@@ -129,7 +165,15 @@ export default function ImageScroller(props) {
                height={192}
                className={classes.image}
                fixed
+                style={{ transform: `rotate(${rotation}deg)` }}


Applying the style attribute here overrides the value inside the OptimizedImage component, causing the image to become squashed.
Instead, you could add a rotation or transform prop to OptimizedImage and add the transform to the style within that component.

nmcharlton · 2021-08-08T10:50:54Z

src/components/ImageScroller.js

+    position: 'absolute',
+    bottom: 0,
+    right: 0,
+    width: 25,


I think the button needs to be larger (try 32px).

nmcharlton · 2021-08-08T10:53:08Z

src/components/ImageScroller.js

              />
+              <IconButton


A Fab is more appropriate here, and the default styling is better than an IconButton.

nmcharlton · 2021-08-08T10:58:35Z

src/components/ImageScroller.js

@@ -118,7 +154,7 @@ export default function ImageScroller(props) {
          images.slice(0, maxImages).map((img, idx) => (
            <Card
              key={`${idx}_${img}`}
-              onClick={() => onSelectImage(img)}
+              onClick={() => handleDataChange(img)}


Clicking the image shouldn't trigger rotation, just select the image.
Only clicking the rotate button should trigger rotation.

nmcharlton · 2021-08-08T11:04:54Z

src/components/ImageScroller.js

              />
+              <IconButton


We should only show the rotation button on (and apply to the rotation to) the selected image.
Otherwise it looks a bit messy with multiple images:

Screen.Recording.2021-08-08.at.12.03.36.mov

nmcharlton · 2021-08-08T11:09:03Z

src/components/ImageScroller.js

@@ -103,6 +126,19 @@ export default function ImageScroller(props) {
    });
  }

+  function handleDataChange(img) {
+    if (img) {


Doing the selection and rotation at the same time doesn't work. Only the rotation is applied.
Selection and rotation should be two separate actions.

nmcharlton · 2021-08-08T11:11:48Z

src/components/EditPlanter.js

@@ -90,17 +90,13 @@ const EditPlanter = (props) => {
      return newPlanter[key] !== planter[key];
    });

-    if (changed) {
+    if (key === 'imageRotation' || changed) {


This causes the Save button to be enabled if there is a non-zero rotation, even if nothing has changed.

nmcharlton · 2021-08-08T11:12:35Z

src/components/ImageScroller.js

+    width: 25,
+    height: 25,
+    color: 'white',
+    backgroundColor: 'black',


If you use a Fab component, there's no need to override the default colors.

nmcharlton · 2021-08-08T11:14:07Z

src/components/ImageScroller.js

+              <IconButton
+                className={classes.clickRotate}
+                onClick={() => handleDataChange(null)}
+                style={{ transform: `rotateY(180deg)` }}


It's just the icon we want to rotate, not the button.

nmcharlton · 2021-08-09T20:48:55Z

@tranquanghuy0801 For info, while having a look at the latest updates I noticed a bug in the api (fixed in Greenstand/treetracker-admin-api#587) whereby imageRotation was required in a planter PATCH.

The fix has now been deployed to dev and test.

tranquanghuy0801 · 2021-08-10T00:19:00Z

@nmcharlton Yeah thanks for fixing that issue. I will check my code again later today.

tranquanghuy0801 · 2021-08-10T10:03:47Z

@nmcharlton Now I am getting stuck at this. As the Card image and the rotation icon both have the onClick props, therefore when I click the icon, it will also execute the onClick of the Card image, which will overwrite the planterUpdate variable

planterUpdate = {imageRotation: 90} --> planterUpdate = {imageUrl: ''}

This will make the "Save" button become inactive even you click rotate icon. Hope this make sense. Do you have any thought on this case?

tranquanghuy0801 · 2021-08-10T10:13:04Z

@nmcharlton that's alright. I knew how to fix that. Check my latest commits.

nmcharlton

Thanks @tranquanghuy0801. This looks a lot better, but I still see a couple of issues:

The rotation should only be applied to the selected image in the Edit Planter dialog, but is applied to all of them. Search planter ID 323 in the Planters tool for an example.
Rotated images have gaps at the top and bottom in the Edit Planter dialog, and overlap text in the Planter Detail drawer (see video below). The image should fill but not overflow the container.
If you edit the planter from the Verify tool, the Planter Detail does not get updated straight away. This is a more general problem, and not specific to image rotation, so could be handled under a separate PR if you don't want to fix it here.

Screen.Recording.2021-08-14.at.14.02.18.mov

tranquanghuy0801 · 2021-08-15T13:03:55Z

@nmcharlton
Thanks for your feedback. I fixed the issue 1 & 2. About the issue 3 you mentioned, you can create another issue for this and assign that to me.

nmcharlton

Sorry to keep asking for changes – this is a more complex problem than I'd realised!

nmcharlton · 2021-08-16T18:21:41Z

src/components/ImageScroller.js

@@ -30,7 +30,7 @@ const useStyle = makeStyles((theme) => ({
    textAlign: 'center',
  },
  imageCard: {
-    height: '100%',
+    height: '84%',


Let's keep it simple and make these images square.

nmcharlton · 2021-08-16T18:24:48Z

src/components/ImageScroller.js

+    objRotation[selectedIndex] = imageRotation;
+  } else {
+    images.map((_, idx) => {
+      objRotation[idx] = imageRotation;


It doesn't really make sense to apply image rotation to all images if none is selected.
Just leave all other images unrotated and apply the rotation to the selected image only.

nmcharlton · 2021-08-16T18:28:42Z

src/components/ImageScroller.js

+      objRotation[idx] = imageRotation;
+    });
+  }
+  let [rotations, setRotations] = useState(objRotation);


I think this is introducing unnecessary complexity.
There's no point in persisting the image rotation for other images in the frontend like this; only one value gets stored for each planter so all other rotations will be lost when they refresh (which is annoying for a user).

nmcharlton · 2021-08-16T18:31:53Z

src/components/ImageScroller.js

@@ -147,14 +157,14 @@ export default function ImageScroller(props) {
                height={192}
                className={classes.image}
                fixed
-                rotation={rotation}
+                rotation={rotations[idx] ? rotations[idx] : imageRotation}


I'd suggest reverting back to the single value, but only applying it if this is the selected image.
When the user selects a new image, just reset the rotation to zero.

nmcharlton · 2021-08-16T18:33:27Z

src/components/PlanterDetail.js

@@ -108,7 +105,14 @@ const PlanterDetail = (props) => {
  return (
    <React.Fragment>
      <Drawer anchor="right" open={props.open} onClose={props.onClose}>
-        <Grid className={classes.root}>
+        <Grid


Let's make the container and image square to avoid this awkwardness and inconsistency between rotated and unrotated images.

The planter card can also be updated to use a square image.

tranquanghuy0801 · 2021-08-17T13:11:59Z

@nmcharlton

I am a bit struggling to remove the white gaps when setting the rotation of image = 90 or 270 deg if I set the images square as you said in Edit Planter dialog. Do you have any suggestions at this?

nmcharlton · 2021-08-18T15:42:51Z

@nmcharlton

I am a bit struggling to remove the white gaps when setting the rotation of image = 90 or 270 deg if I set the images square as you said in Edit Planter dialog. Do you have any suggestions at this?

@tranquanghuy0801 It seems to be a quirk of the way rotated elements are rendered, presumably due to some rounding in the transform calculation.
As a workaround I'd suggest increasing the height and width of the image slightly (just make sure you add a comment explaining the css. This appears to work in the OptimizedImage style property:

          width: 'calc(100% + 1px)',
          height: 'calc(100% + 1px)',

feat: planter selfie rotation

a636045

tranquanghuy0801 requested review from gwynndp, luacmartins and nmcharlton as code owners August 4, 2021 13:23

tranquanghuy0801 force-pushed the fix-81 branch from 45e431e to a636045 Compare August 4, 2021 13:36

nmcharlton requested changes Aug 8, 2021

View reviewed changes

fix: re-format the apply rotation planter image

1863c6b

fix: fix the overwrite on-click rotation

624f386

nmcharlton requested changes Aug 14, 2021

View reviewed changes

fix: fix the overflow image rotation

d2de60e

tranquanghuy0801 force-pushed the fix-81 branch from 8548b61 to d2de60e Compare August 15, 2021 13:16

nmcharlton requested changes Aug 16, 2021

View reviewed changes

tranquanghuy0801 and others added 2 commits August 23, 2021 19:53

fix: update the rotation for multiple images

6c2305f

fix: make all planter images square for ease of rotation

ee65107

nmcharlton approved these changes Sep 15, 2021

View reviewed changes

nmcharlton merged commit f307700 into Greenstand:master Sep 15, 2021

tranquanghuy0801 deleted the fix-81 branch October 13, 2021 11:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: planter selfie rotation #142

feat: planter selfie rotation #142

tranquanghuy0801 commented Aug 4, 2021

nmcharlton left a comment

nmcharlton Aug 8, 2021

nmcharlton Aug 8, 2021

nmcharlton Aug 8, 2021

nmcharlton Aug 8, 2021

nmcharlton Aug 8, 2021

nmcharlton Aug 8, 2021

nmcharlton Aug 8, 2021

nmcharlton Aug 8, 2021

nmcharlton Aug 8, 2021

nmcharlton Aug 8, 2021

nmcharlton commented Aug 9, 2021

tranquanghuy0801 commented Aug 10, 2021

tranquanghuy0801 commented Aug 10, 2021 •

edited

Loading

tranquanghuy0801 commented Aug 10, 2021

nmcharlton left a comment

tranquanghuy0801 commented Aug 15, 2021

nmcharlton left a comment

nmcharlton Aug 16, 2021

nmcharlton Aug 16, 2021

nmcharlton Aug 16, 2021

nmcharlton Aug 16, 2021

nmcharlton Aug 16, 2021

tranquanghuy0801 commented Aug 17, 2021 •

edited

Loading

nmcharlton commented Aug 18, 2021

feat: planter selfie rotation #142

feat: planter selfie rotation #142

Conversation

tranquanghuy0801 commented Aug 4, 2021

nmcharlton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmcharlton commented Aug 9, 2021

tranquanghuy0801 commented Aug 10, 2021

tranquanghuy0801 commented Aug 10, 2021 • edited Loading

tranquanghuy0801 commented Aug 10, 2021

nmcharlton left a comment

Choose a reason for hiding this comment

tranquanghuy0801 commented Aug 15, 2021

nmcharlton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tranquanghuy0801 commented Aug 17, 2021 • edited Loading

nmcharlton commented Aug 18, 2021

tranquanghuy0801 commented Aug 10, 2021 •

edited

Loading

tranquanghuy0801 commented Aug 17, 2021 •

edited

Loading