Feat/albas improvements #644
Conversation
Also organised imports, changed to updating properties, no optional chaining, no nullish coalescing or undefined values unless strictly necessary (will test this later).
…as changed. Parent needs to know that the settings were updated to update view.
…sly checkWithinCropperSizeBounds) - Changed how cropper size bounds are calculated. - Changed naming of scaled cropper state values to internal, as we now check all of them –static too– and they follow different rules. - Added hiding resize squares when static side and changed cursor in non hidden squares. - Deleted resetCropOnAspectRatio change (explained in the rules section of cropperPosition.utils in changes) - Change naming of checkWithinCropperSizeBounds to checkSizeAndPosition, and updated it to cover new behaviour explained in rules (see changes, cropperPosition.utils). - See rules in cropper position and cropper size bounds utils.
… size on resize window
- How it moves when resizing when maintain aspect ratio is true - Can resize when one internal static side is applied
|
I forgot to make and send the videos. I sent changes so cropper can resize when there's one static side (I don't think that needs a video) and changed how it resizes when maintain aspect ratio is true. Before, it would resize from a corner when resizing from a side.
Now, it resizes from the side:
Before, it only resized on x movement when resizing from a corner.
Now, it can resize on x and y movement.
But there's still an area where it doesn't respond. This happens in all corners. I gave up on fixing it.
|
|
Hi there, |
|
Yeah I'm not a fan either. I like some bits though. |
|
left some comments on the code. I would like to separate this into 2 PRs:
I'm keen on the resize changes. That's my thoughts |
|
@bboyle thanks for this :) So the settings was a mistake and I'm changing it back. I take it you want the resizing now? If so I'll look into dividing the PR. I think there was a breaking change involved but I can't remember right now. Need to check. For the resizing to work it has to take into account cropperMinWidth, cropperMinHeight, cropperMaxWidth, cropperMaxHeight, cropperStaticWidth, cropperStaticHeight and mantainAspectRatio. I suspect the breaking change was related to updates I did there. There will be breaking changes with the rest of the changes that I'll be sending in, assuming they're accepted. |
|
No immediate rush. I would like the resize changes, but certainly not an emergency or anything like that. I consider breaking changes to be anything that means a new version used with the same configuration does not work without the developer making additional changes. If a developer can update to the latest version and it keeps working, or works better (as in the case of bug fixes or new features), that's not a breaking change. If it stops working until I make changes to my code as well, that is a breaking change. I think the changes to settings were breaking, because my existing settings would not have worked. Just how I think (or at least, how I interpret "semantic versioning"). |
|
Okay so I checked. I deleted the resetCropOnAspectRatio input which is a breaking change, specially for the person that added it. In retrospect I can see how I really put my foot in it by doing such a drastic change to the settings. With the changes that I'm sending in I'm giving devs more control over how to set and reset the cropper as well as how to position the img. Both of these involve breaking changes – btw I agree with your interpretation of "semantic versioning" – and are also a pretty big behaviour change. I figured better to merge all in one go with a nice explanation so devs know what to change. This way they only have to do this once. The resizing thing was a by-product of me looking at this code so much. But if I hadn't changed the settings so drastically I can see how I could have not deleted that input and the code could probably be merged now (would need to double check). However, I'm going to continue without trying to divide the PR as you have said you are not in a rush for the resizing. I will change the settings back too. If you at some point change your mind, do let me know. |
Hi! Not finished yet. Sending it now so you can start looking at it while I continue working on the next bits.
I changed your state approach to show you what I had in mind. I'm not entirely convinced by it. Just leaving it for now so I can move on with the other stuff.
I think it's important to only use state in image-cropper-component.ts to avoid confusion and not to separate the logic in ngOnChanges as it's already a brain bender. We can move it into a function somewhere, but keep it all together. Not in different functions/class methods.
I cringed at the settingsToUpdate input in my state approach. Seems like it should be a method. Which in a way makes me happy cos having only update transform and update cropper as methods was feeling weird to me. This is just food for thought though. I think we should talk more seriously about state once I send you everything and you read through it.
I deleted all the inputs so we can work on a clear-end result. I'll add them back at the end so developers can use the old and new approach like you said.
I've also changed it back to methods updating state values instead of returning the values which are then passed on to state in main. I want to test both approaches at the end to see if there's a difference. I suspect I might be wrong but I still want to test. I understand what you wanted and I like pure functions too. I just want to see if in this case, where ultimately I'd like the lib to be able to do things like scaling when resizing, simply updating the values is better. Same with undefined values and nullish coalescing or ||.
Which brings me to the next part. I saw you were trying to get rid of hammer. How's it going? Hammer seems to be terrible for performance so that would be awesome! Also you can unify a lot of the pointer events logic and put it in utils, further decluttering image-cropper-component.ts.
Any questions, I'm here. Chao chao for now :)
[EDIT]: Forgot, I'll add your tests back too.