-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Use composer state as source of truth for embeds/links on publish #5606
Conversation
|
Co-authored-by: Mary <[email protected]>
Co-authored-by: Mary <[email protected]>
quote = { | ||
type: 'link', | ||
uri: initQuoteUri, | ||
// TODO: Consider passing the app url directly. |
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.
Seems fine as is, but leaving a note here in case you wanted to remember to come back here.
edit: lots of other todos up above so assume these are already noted 👍
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.
this one actually seems like it's a bit more convenient to leave as is, we're relying on initQuote.uri
being an at://
uri in a different place
Tested composer + resulting post with:
Bugs (not regressions):
Notably though, I think with our current setup you can't have both a gif and a link embed, since they both use the same type in the record. IMO we should address that with a lexicon addition/change but we'll see. However, the quote-gif combo should work correctly so we need to fix that. |
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.
Works well from the testing above ^. A few bugs, only one of which we can actually fix and already exists in prod. That can come in a separate PR. Beautiful!
Ah, looks like in the final PR that bug is fixed. So really just the weird gif and link thing we can't solve with frontend alone. |
* origin/main: (267 commits) Nicer error message for disabled quotes (#5644) Update neue nux date (#5643) Make alt text scrollable on native (#5642) Move remaining composer state into reducer (#5623) Fetch link previews from RQ (#5608) Use composer state as source of truth for embeds/links on publish (#5606) Update Indonesian translation (#5316) [Video] Add dimension info to share intent pt.2 (#5640) [Video] Prevent screen from dimming while in full screen (#5637) [Video] Add dimension info to share intent (#5639) [Video] Revert safari hackfix (#5367) Swipeable to delete chat, custom swipeable (#5614) equal spacing on displayname/handle (#5636) Update Japanese translation (#5374) Update catalana messages.po (#5380) Update and inconsistency fixes to pt-br translation (#5436) Tweak #5522 (#5635) Update Korean localization (#5401) Update Chinese localization (#5433) Add Cantonese Localization (#5479) ...
This is the equivalent of #5595, but for embeds, links, and GIFs.
With this PR, we're not using the old
Composer
state variables likequote
,extLink
, orextGif
for posting. Instead, we resolve links/records from thecomposerState
managed by the new composer reducer.This also means that
useExternalLinkFetch
is no longer used as a source of truth for posting. It's still used for previews though. In a follow up, I'll fix that up.Note that this is a slight performance regression. That's because resolving is always done from scratch during post, and there is no caching applied. (Effectively, link meta and previews now get fetched twice — once on post and once on publish.) This will be fixed in a follow-up PR where I'll move resolving into RQ and hook it up to both places.
I won't merge this on its own without the follow-up PR that fixes perf. So this is mostly just for isolated review.
Test Plan
Try combinations of quotes, images, GIFs, other records, and link embeds. Verify the posted result makes sense.