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

embedded tweet refactoring jnakayama 2024 10 10 #211

Merged
merged 9 commits into from
Oct 12, 2024

Conversation

JohannesNakayama
Copy link
Member

  • embedded tweet refactoring
  • display embedded tweet on artefact page

@JohannesNakayama JohannesNakayama marked this pull request as ready for review October 11, 2024 09:59
@JohannesNakayama JohannesNakayama added the ready-to-merge-rebase Automatically merge with rebase label Oct 11, 2024
Copy link
Contributor

@johnwarden johnwarden left a comment

Choose a reason for hiding this comment

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

Looks great. Some small comments:

  • Small issue with error-handling logic. For example, if you submit a URL that looks valid, but with a non-existent ID, it just says "loading" instead of "Tweet not found"
  • On the quote/$quoteId page, the embedded Tweet is shown above the tabs with the extracted claims etc. You have to scroll down to see anything else. I think the "Retrieved..." text and the tabs should actually go above the embedded tweet.

@mergify mergify bot merged commit e60fda0 into main Oct 12, 2024
4 checks passed
@mergify mergify bot deleted the embedded-tweet-refactoring-jnakayama-2024-10-10 branch October 12, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge-rebase Automatically merge with rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants