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

add support for quoted tweets #117

Merged
merged 3 commits into from
Aug 11, 2023
Merged

add support for quoted tweets #117

merged 3 commits into from
Aug 11, 2023

Conversation

ilyichv
Copy link
Contributor

@ilyichv ilyichv commented Aug 3, 2023

Add support for quoted tweets.

Fixes #111

Comment on lines 9 to 19
const getSkeletonStyle = (media: MediaDetails, itemCount: number) => {
let paddingBottom = 100 // default of 1x1

// if we have 2 items, use 8x9
if (itemCount === 2) paddingBottom = 112.5

return {
width: media.type === 'photo' ? undefined : 'unset',
paddingBottom: `${paddingBottom}%`,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something, but I wouldn't think that this getSkeletonStyle method would need to differ at all from the one used in tweet-media.tsx. The layout of media should have the same layout as a normal tweet.

That makes me then wonder why we need a separate tweet-quoted-tweet-media.tsx at all? The minor styling differences might be able to be accounted for in the main media component.

Copy link
Contributor Author

@ilyichv ilyichv Aug 4, 2023

Choose a reason for hiding this comment

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

You might be right. I thought that quoted tweets were handled differently because of this example but actually there are some mismatches between react-tweet image handling and twitter embeds. Take this for instance, ratio is 1:1.

Copy link
Contributor Author

@ilyichv ilyichv Aug 4, 2023

Choose a reason for hiding this comment

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

Also its behaviour with 4 images is weird, I can leave it as it is for the moment and open another issue for aligning its behaviour with twitter one.

@ilyichv
Copy link
Contributor Author

ilyichv commented Aug 4, 2023

@tywayne just updated

@leerob
Copy link
Member

leerob commented Aug 8, 2023

Could we see some examples to validate it's working?

@ilyichv
Copy link
Contributor Author

ilyichv commented Aug 8, 2023

For sure, below some of them.

image image image

}
switch (user.verified_type) {
case 'Government':
icon = <VerifiedGovernment />
Copy link
Member

Choose a reason for hiding this comment

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

We'll probably want to do X Verified Businesses at some point, where it shows the affiliate logos

Copy link
Member

@leerob leerob left a comment

Choose a reason for hiding this comment

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

Seems good, will defer to @lfades for final call

Copy link
Member

@lfades lfades left a comment

Choose a reason for hiding this comment

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

Tested it out locally and looks good. Thank you!

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.

Support quoted Tweets
4 participants