-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add alt text limit to image dialog #5611
Conversation
|
src/view/com/composer/GifAltText.tsx
Outdated
}, [onSubmit, altText]) | ||
const t = useTheme() | ||
const {_, i18n} = useLingui() | ||
const [altText, setAltText] = useState(altTextRef.current) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn’t read refs during render.
this might be ok but why are we using a ref here? isn’t this a controlled input anyway?
onChange, | ||
Portal, | ||
}: Props): React.ReactNode => { | ||
const altTextRef = React.useRef<string>(image.alt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, another ref. i’m worrying about proliferation of refs — why are we adding them? they should only be used as escape hatches and i don’t yet see what we’re escaping from. generally adding them for normal data flow needs to have some kind of explanation so we don’t miss this in reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let’s make sure we have a solid justification for adding refs — easy way to cause bugs if we’re not careful
example in gif dialog in 13d195a |
Updates alt text limit to 2k characters for both images and GIFs + updates the UI with our char counter.