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

✨ Show embedded list in post card #138

Merged
merged 2 commits into from
Jul 3, 2024
Merged

✨ Show embedded list in post card #138

merged 2 commits into from
Jul 3, 2024

Conversation

foysalit
Copy link
Contributor

@foysalit foysalit commented Jun 26, 2024

Screenshot 2024-06-27 at 00 46 03

Before

Screenshot 2024-06-27 at 00 39 40 > After

Copy link

render bot commented Jun 26, 2024

Copy link

render bot commented Jun 26, 2024

@foysalit foysalit requested a review from devinivy July 1, 2024 07:51
Comment on lines +339 to +340
did: `${did}`,
rkey: `${rkey}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a way of casting did and rkey into a string? If so, could it end-up casting them to something we don't intend like 'null' or 'undefined'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

record.uri is always present and parsing the uri should always return did and rkey for isViewBlocked AFAICT so this is just appeasing the ts linter.

Comment on lines +349 to +356
<ProfileAvatar
profile={{
avatar: embed.record.avatar,
did: embed.record.creator.did,
handle: embed.record.name,
}}
className="w-6 h-6 rounded-full"
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be worth renaming the ProfileAvatar component or extracting out a more generic component from it? I notice that we're "tricking" it into thinking it's displaying profile information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm.. idk, it's minimal lift but I also don't see avatars being shown for anything other than users/profiles so don't think it it matters a lot either way?

Comment on lines +363 to +365
<>
<span className="font-bold">{embed.record.name}</span>
</>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be able to get rid of this fragment.

@foysalit
Copy link
Contributor Author

foysalit commented Jul 3, 2024

I don't disagree strongly with any of the suggestions above so might end up making those changes in a future iteration but don't think any of those are blockers so going to ahead and merge.

@foysalit foysalit merged commit 68c459a into main Jul 3, 2024
3 checks passed
@matthieusieben matthieusieben deleted the list-embed branch November 15, 2024 14:54
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