Skip to content

Snap cropping when close to nothing (BL-13952)#6724

Merged
andrew-polk merged 1 commit intoBloomBooks:Version6.1from
JohnThomson:snapOnZeroCrop
Oct 25, 2024
Merged

Snap cropping when close to nothing (BL-13952)#6724
andrew-polk merged 1 commit intoBloomBooks:Version6.1from
JohnThomson:snapOnZeroCrop

Conversation

@JohnThomson
Copy link
Copy Markdown
Contributor

@JohnThomson JohnThomson commented Oct 24, 2024

This change is Reviewable

Copy link
Copy Markdown
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JohnThomson)


src/BloomBrowserUI/bookEdit/js/bubbleManager.ts line 1601 at r1 (raw file):

                    break;
                case "e":
                    // LHS is where the right of the image is. RHS is where the right of the overlay is.

Sorry; I don't understand these comments.
I don't see LHS or RHS except in these comments, but it seems like the comment is supposed to be telling me what they mean.

I wonder if it is worth extracting this logic to a function called shouldCropSnapBeDisabled and giving it a comment explaining, generally, why we want it disabled vs enabled.
But on second thought, maybe that implies too much (namely that you've encapsulated all such logic in one place) since you have lots more logic below about when it should be disabled/enabled.

Copy link
Copy Markdown
Contributor Author

@JohnThomson JohnThomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrew-polk)


src/BloomBrowserUI/bookEdit/js/bubbleManager.ts line 1601 at r1 (raw file):

Previously, andrew-polk wrote…

Sorry; I don't understand these comments.
I don't see LHS or RHS except in these comments, but it seems like the comment is supposed to be telling me what they mean.

I wonder if it is worth extracting this logic to a function called shouldCropSnapBeDisabled and giving it a comment explaining, generally, why we want it disabled vs enabled.
But on second thought, maybe that implies too much (namely that you've encapsulated all such logic in one place) since you have lots more logic below about when it should be disabled/enabled.

I think when I wrote that I was comparing a left-hand-side expression with a right-hand-side. Then Math.abs got involved, and then prettier, and now it's not two sides but five or six lines. I reworded and added some more comments.
I don't think the nature of what John asked for lends itself to handling crop snapping all in one place.

Copy link
Copy Markdown
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @JohnThomson)

@andrew-polk andrew-polk merged commit 24c1375 into BloomBooks:Version6.1 Oct 25, 2024
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.

2 participants