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

feat: add BlueskyPost #167

Merged
merged 19 commits into from
Nov 11, 2024
Merged

feat: add BlueskyPost #167

merged 19 commits into from
Nov 11, 2024

Conversation

ascorbic
Copy link
Contributor

@ascorbic ascorbic commented Nov 4, 2024

This adds a Bluesky Post embed. It's styled like the official one (which is Preact and Tailwind), but is a fully static re-implementation.

Copy link

changeset-bot bot commented Nov 4, 2024

🦋 Changeset detected

Latest commit: 5146bf8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@astro-community/astro-embed-integration Minor
@astro-community/astro-embed-bluesky Minor
astro-embed Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Nov 4, 2024

Deploy Preview for astro-embed ready!

Name Link
🔨 Latest commit 5146bf8
🔍 Latest deploy log https://app.netlify.com/sites/astro-embed/deploys/67321ac7e1bdfd0008af0709
😎 Deploy Preview https://deploy-preview-167--astro-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ascorbic ascorbic changed the title wip: add BlueskyPost feat: add BlueskyPost Nov 5, 2024
@ascorbic ascorbic marked this pull request as ready for review November 5, 2024 22:25
Copy link
Owner

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thanks @ascorbic this looks great! 🙌

I’m a bit nervous about containing the styles/avoiding a site’s styles from clashing, but don’t think the Declarative Shadow DOM approach we use in the <BaselineStatus> component is viable here, so we’ll have to roll with it. (This is partly why I’ve mostly chosen “low style” for the other embeds — even when Twitter API access was viable, we shipped very minimal styles. On the one hand that lets the content blend into a site’s styles, and on the other it got me off the hook from worrying about this kind of thing 😁)

Left a few questions and suggestions!

packages/astro-embed-bluesky/src/styles.css Outdated Show resolved Hide resolved
packages/astro-embed-bluesky/src/styles.css Outdated Show resolved Hide resolved
packages/astro-embed-bluesky/src/utils.ts Outdated Show resolved Hide resolved
packages/astro-embed-bluesky/src/styles.css Outdated Show resolved Hide resolved
demo/src/pages/bluesky.astro Show resolved Hide resolved
demo/src/pages/bluesky.astro Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

Docs here look good. We should also document the styling API (which may involve deciding which if not all the custom properties are considered “public”). But that could also happen in a later PR if we want to get this tested in some real world scenarios before committing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think they should be public. If people want to poke around and style them then they can, but I think we public API should just be to have them look like the official embed.

@ascorbic
Copy link
Contributor Author

Thanks for the review! I've refactored the styles quite a bit. There aren't any classes in styles.css now: it's just var declarations. Those are now all scoped to the post container, and are also prefixed. I've changed avatar to be a component.

Those rendering issues were a nightmare! I thought I'd caught them all. They're all related to the way that Bluesky nests all of the records in different ways in different post types. I'll look at what's going on there.

@ascorbic
Copy link
Contributor Author

After a lot of refactoring it turns out the rendering issue wasn't related to the nesting logic at all: it was just that it was trying to next a tags, which the browser was helpfully "fixing". The refactor is an improvement though!

@ascorbic ascorbic requested a review from delucis November 10, 2024 19:37
@ascorbic
Copy link
Contributor Author

The Netlify failure seems unrelated?


{embed && <Embed embed={embed} postUrl={postUrl} />}
<a href={postUrl} class="timestamp">
{formatter.format(new Date(record.createdAt ?? ''))}
Copy link
Owner

Choose a reason for hiding this comment

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

Added some tests which failed in CI because of a different timezone compared to running locally which made me think again about this. I now remember we hit the same question with the old Twitter embed — if we render a timestamp on the server and therefore in the server’s time zone, that can be confusing because it is not local to the user (unlike the client-rendered components).

I think for Twitter I just decided to display a date with no time IIRC (still has a chance of being off, but it’s not as likely).

There’s also the option of some minimal JS hydration, e.g. to render something like this which updates the time to the user timezone:

<bluesky-time>
  <time datetime={record.createdAt?.toISOString()}>
    {formatter.format(new Date(record.createdAt ?? ''))}
  </time>
</bluesky-time>

<script>
const formatter = new Intl.DateTimeFormat('en-US', { dateStyle: 'long', timeStyle: 'short' });

customElements.define('bluesky-time', class BlueskyTime extends HTMLElement {
	connectedCallback() {
		try {
			const time = this.querySelector('time');
			const datetime = time?.getAttribute('datetime');
			if (!datetime) return;
			const date = new Date(datetime);
			time.textContent = formatter.format(date);
		} catch {}
	}
});
</script>

Does away with the zero-JS claim though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NEVER

@delucis
Copy link
Owner

delucis commented Nov 11, 2024

The Netlify failure seems unrelated?

Yeah… probably my safeGet() refactor — a 404 like that should not error in theory

Edit: Ah, misread the logs. That’s not the actual failure. Might just be a fluke?

@ascorbic
Copy link
Contributor Author

Yeah, possibly just flakiness. If you try a redeploy it might work

@delucis
Copy link
Owner

delucis commented Nov 11, 2024

OK, curiously reverting 7699491 makes Netlify builds succeed again, but the build for that commit passed. I’d guess something about the re-use of the responses in the cached version perhaps triggered that terminated error? I guess we can do without the caching for now. (Shame because at least for the handle resolving, I’d guess there are likely quite a few cache hits.)

@ascorbic
Copy link
Contributor Author

Ah I think I know the problem. You can't re-use a Response, because as soon as you call json() or whatever, it consumes the stream. If you try to call it again you'll get a stream error. It would need to return a clone each time.

@delucis
Copy link
Owner

delucis commented Nov 11, 2024

It would need to return a clone each time.

Yeah, that’s what my implementation was intending to be doing, but I probably messed it up somehow. I just tried an alternative approach using ultrafetch, which seems to work, so may make a follow-up PR to clean up some of that code.

@ascorbic
Copy link
Contributor Author

Yeah I think the problem was that unless I misunderstood it, it was only cloning the first time, and then caching the cloned copy and not cloning it again on each get.

Copy link
Owner

@delucis delucis left a comment

Choose a reason for hiding this comment

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

OK, let’s do this!

I added one last change to let the post resolver fail gracefully instead of erroring out for unknown IDs. We can revisit fetch caching later.

@delucis delucis merged commit 22779f0 into delucis:main Nov 11, 2024
5 checks passed
@github-actions github-actions bot mentioned this pull request Nov 11, 2024
@ascorbic ascorbic deleted the bluesky branch November 11, 2024 15:06
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.

2 participants