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

Split GifEmbed props #5615

Closed
wants to merge 1 commit into from
Closed

Split GifEmbed props #5615

wants to merge 1 commit into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 5, 2024

This would help me in an upcoming refactor. Just splitting the props so it doesn't always accept the whole link object.

Test Plan

Verify GIFs with explicit ALT and implicit ALT still show up (and both have alt in DOM, but only explicit one shows badge).

Verify editor still works.

@arcalinea arcalinea temporarily deployed to ref-gif-embed-props - social-app PR #5615 October 5, 2024 11:05 — with Render Destroyed
@gaearon gaearon marked this pull request as ready for review October 5, 2024 11:05
link={link}
thumb={link.thumb}
altText={altText}
isPreferredAltText={true}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note these can just be altText and true now because we're always passing down the "preferred" version. (In any case they don't really matter because it's a preview and has hideAlt. Arguably maybe it shouldn't get any alt at all.)

@gaearon gaearon requested review from mozzius and haileyok October 5, 2024 11:07
Copy link

github-actions bot commented Oct 5, 2024

Old size New size Diff
7.92 MB 7.92 MB 228 B (0.00%)

@surfdude29
Copy link
Contributor

Verify GIFs with explicit ALT and implicit ALT still show up (and both have alt in DOM, but only explicit one shows badge).

Until I just saw this, I was under the mistaken impression that GIFs that didn't display the ALT badge didn't have alt in DOM, so it's good to know that they actually do.

Given that this might be confusing other people too (see #4918), is it planned as part of your refactor to show the badge for GIFs with implicit ALT too? I think this would be helpful and less confusing for users.

@surfdude29
Copy link
Contributor

surfdude29 commented Oct 5, 2024

Also, while you're working on the GIF alt composer, would it be possible to squeeze in a fix for #4225? 🙏

(although it's possible that's already been fixed on iOS by #5611?)

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 5, 2024

This PR is really a drive-by refactor for unrelated things I'm working on so I'm not taking a super close look at how it behaves.

Given that this might be confusing other people too (see #4918), is it planned as part of your refactor to show the badge for GIFs with implicit ALT too?

With the behavior currently in main, it won't show the badge until you open the "alt" popup, but once you do (and close it), that alt will be considered explicit. Which is slightly confusing in a different way but probably ok.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 5, 2024

Meh I'll just roll it all into one PR.

@gaearon gaearon closed this Oct 5, 2024
@gaearon gaearon deleted the ref-gif-embed-props branch October 5, 2024 13:50
@surfdude29
Copy link
Contributor

surfdude29 commented Oct 5, 2024

This PR is really a drive-by refactor for unrelated things I'm working on so I'm not taking a super close look at how it behaves.

Ah fair enough 👍

Given that this might be confusing other people too (see #4918), is it planned as part of your refactor to show the badge for GIFs with implicit ALT too?

With the behavior currently in main, it won't show the badge until you open the "alt" popup, but once you do (and close it), that alt will be considered explicit. Which is slightly confusing in a different way but probably ok.

Yeah the behavior on main is a big improvement over 1.91.1.

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