Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust Image Cropper handle (focal point) #1021

Open
wants to merge 93 commits into
base: main
Choose a base branch
from

Conversation

bjarnef
Copy link
Contributor

@bjarnef bjarnef commented Nov 26, 2023

Description

This PR adjust focal point (handle) in Image Cropper to be more consistent with handle in Color Area.
umbraco/Umbraco-CMS#16672

It moves the focal point based on x and y coordinates in the container on mouse click or touch.
Furthermore it moves in the handle / focal point on keyboard arrows, while holding down Shift key allows to fine-tune position.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Motivation and context

To re-use some existing logic and make the UI/UX consistent.

How to test?

Screenshots (if appropriate)

chrome_2tFi6FEudB.mp4

Checklist

  • If my change requires a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@bjarnef bjarnef marked this pull request as ready for review November 28, 2023 19:47
@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 28, 2023

There a minor issue when resetting focal point, where it also need to reset the coords like it does on image loaded here:
https://github.com/umbraco/Umbraco.CMS.Backoffice/pull/1021/files#diff-edf1088805f907d36474f517086a2f1a7b8c0f07f1aeaa66d6d1861da99a6d12R72-R74

It am not sure the disabled property is something we need, but I think it would be a nice feature.
In the current backoffice if a property editor is readonly, I think the focal point is just hidden.

If think it may be useful to have a feature to show the focal point, but maybe some editors should allow the move it or in a special context like split view or when not active language variant.

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 28, 2023

@iOvergaard @JesmoDev please have a look 😎

@nielslyngsoe
Copy link
Member

nielslyngsoe commented Nov 28, 2023

Hi @bjarnef looks good.

I also see that you are trying to fix a few other things, like keyboard navigation etc. Thats cool, would it be possible to finish it in near future?

Otherwise, I would recommend splitting your PRs up. Small PRs that are easy to review can sometimes be really good as they are fast to review and we can then get them merged in. In this way, we can keep the list of open PRs to a minimum. (as I was up for merging this, but its not done in its current state, just the change of the focus point look) :-)

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 28, 2023

@nielslyngsoe it should be ready to review, but any feedback are welcome 😉
I am still learning by doing 🤭😅😎

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 28, 2023

Do we have any way to know when reset focal point happens? It seems firstUpdated() happens on init and updated() each time focal point is moved, but we only want to update coords to center when focal point reset is triggered.

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 28, 2023

I found a workaround to update coords if focal point is updated/reset to center using the reset button.
11666ac

@nielslyngsoe
Copy link
Member

@bjarnef yes, that is one solution. But the one we mainly use is to append setter and getter methods to the property itself. You should be able to fairly simple find a lot of solutions doing so. A typical one is the value and config property on any Property Editor UI.

@bjarnef
Copy link
Contributor Author

bjarnef commented Nov 29, 2023

@nielslyngsoe I tried the following instead. 03bcee2

A bit similar to the UmbInputImageCropperElement here:

However to make #resetCoords() working it need to happen after image has loaded and CSS applied here to get the rendered size.
https://github.com/umbraco/Umbraco.CMS.Backoffice/pull/1021/files#diff-edf1088805f907d36474f517086a2f1a7b8c0f07f1aeaa66d6d1861da99a6d12R92

Or call #setFocalPointStyle() but it requires focalPointElement to be found via query which seems to happen after property value setter.

@bjarnef
Copy link
Contributor Author

bjarnef commented Dec 12, 2023

@JesmoDev this probably conflicts a bit with changes in #1052

This PR makes the Image Cropper handle works using keyboard navigation and use same logic as color picker handle.

I could use some help with implementation of the property as @nielslyngsoe suggested - see last commit.

@bjarnef
Copy link
Contributor Author

bjarnef commented Jan 10, 2024

@iOvergaard do you know how we can solve the last part?
I think it would be great to use same handle as in Color Picker and make it accessible as well.

… feature/image-cropper-handle

# Conflicts:
#	src/packages/media/media/components/input-image-cropper/image-cropper-focus-setter.element.ts
@bjarnef
Copy link
Contributor Author

bjarnef commented Jul 26, 2024

@iOvergaard feel free to make additional commits to this PR :)

I noticed some other behavioural oddities.

Another minor issue I noticed is when opening developer console the image isn't re-calculated. Not a big issue, but I think it would similar a virtual keypad open on tablet. Anyway it worked nicer in old backoffice :)

image

image

When not in crop mode (at Media "crop" menu item), we probably also need checkered background as before to see a white logo/image?

image

@iOvergaard
Copy link
Collaborator

@bjarnef All good proposals. Would you add them as separate issues, please?

Change media item allow me to select a folder (it wasn't possible in old backoffice)

This one we are aware of, we currently have no way of detecting if a media item is actually a folder - all media items can contain subitems, but obviously does not make sense to select one if it itself doesn't have an umbracoFile. We need more advanced checks for that.

@bjarnef bjarnef requested a review from iOvergaard July 28, 2024 12:50
@bjarnef
Copy link
Contributor Author

bjarnef commented Jul 28, 2024

@iOvergaard can you have another look at latest changes. I think there are other enhancements, we can fix in other PRs.
I added two new custom events, but I think currently these introduce breaking changes and they need to have event name change? But if developers already implemented this it probably wouldn't work reacting on CustomEvent anyway.

… feature/image-cropper-handle

# Conflicts:
#	src/packages/media/media/components/input-image-cropper/image-cropper-preview.element.ts
#	src/packages/media/media/components/input-image-cropper/types.ts
#	src/packages/media/media/property-editors/media-picker/index.ts
@bjarnef
Copy link
Contributor Author

bjarnef commented Jul 29, 2024

@iOvergaard does the events need args?: EventInit in constructor?
It is used 3 places in codebase, which has following properties: bubbles, composed and cancelable.

Does it mean when set it override these?

{ bubbles: true, composed: false, cancelable: false }

e.g.

super(UmbSelectionChangeEvent.TYPE, { bubbles: true, composed: false, cancelable: false, ...args });

Copy link

sonarcloud bot commented Sep 3, 2024

@bjarnef
Copy link
Contributor Author

bjarnef commented Sep 3, 2024

@iOvergaard can you review this with the latest changes. It would be great to close this one.

@bjarnef
Copy link
Contributor Author

bjarnef commented Sep 19, 2024

@iOvergaard did you see the latest comments/ commits? It would be great to finish this one 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants