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

New crop image dialog #6002

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

New crop image dialog #6002

wants to merge 3 commits into from

Conversation

mozzius
Copy link
Member

@mozzius mozzius commented Oct 29, 2024

Motivation: remove the use of unstable__openModal in the edit banner/avatar flow on web and move to the new dialogs (since the old ones are broken)

This introduces a new CropImageDialog which has an promise-based API for opening a dialog.

There is an existing ALF dialog for EditImageDialog - however, this doesn't have an API suitable for the edit banner/avatar flow

const [ref, cropImage] = useImageCropperControl()

return (
  <>
    <Button onPress={async () => {
      const res = await cropImage(image)
      // do something with `res`
    }} />
    <CropImageDialog controlRef={ref} />
  </>
)

Test plan

Check that you can edit your avatar/banner on web

Copy link

github-actions bot commented Oct 29, 2024

Old size New size Diff
7.92 MB 7.91 MB -2.06 KB (-0.03%)

@surfdude29
Copy link
Contributor

I tested this out a bit on the preview branch in Safari on iOS 17.7.1 and I wanted to mention a couple of things.

It's possible to get into a state where after you've selected the image, the Edit image dialog appears behind the Edit profile dialog. I thought I had found a way to trigger this easily, but I realised that I had content blockers enabled for that situation, and I'm not sure if that "counts" 😅

Nevertheless, I was able to replicate it with content blockers disabled, it just seemed to occur more randomly (or at least, after a more complex series of steps).

Here's a screenshot:

IMG_9890

Also, as you can see from the screenshot, Edit image shows up in black text even in the Dim theme.

@mozzius mozzius marked this pull request as draft October 29, 2024 20:42
@mozzius
Copy link
Member Author

mozzius commented Oct 29, 2024

Hey @surfdude29, thanks - I'd also noticed this, parking this PR until it can be resolved

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