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

💄 Hide blob list by default and add image viewer #252

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

foysalit
Copy link
Contributor

@foysalit foysalit commented Dec 3, 2024

  • To make more room in the Quick Action Panel, we are now hiding blob list by default, allowing mods to open it as a dropdown when they actually want to action blobs.
  • Added an image viewer to the blob list so that mods can view each individual blob item.
  • Fixed a bug ensuring we open the image that the user clicks in the lightbox view if there are multiple in a post, instead of the first one in the list.
Screenshot 2024-12-03 at 20 31 49

In action panel

Screenshot 2024-12-03 at 20 35 33

In blobs table on profile page

Comment on lines 22 to 31
{!blobs.length ? (
<div className="text-sm text-gray-400">No blobs found</div>
) : (
<BlobListLightbox
blobs={blobs}
authorDid={props.authorDid}
slideIndex={lightboxImageIndex}
onClose={() => setLightboxImageIndex(-1)}
/>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: for the sake of readability I would invert the conditions blobs.length > 0 ? <BlobListLightbox /> : <div />

Copy link
Contributor

@matthieusieben matthieusieben left a comment

Choose a reason for hiding this comment

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

Looks good overall. Some small comments

Comment on lines +9 to +12
const { appview } = useServerConfig()
const cdnUrl = appview?.endsWith('.bsky.app')
? 'https://cdn.bsky.app'
: appview
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to make that part of the server config ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep but im trying to reduce server side touch points. i have a ticket on trello to send it from server.

Comment on lines 55 to 61
console.log(
blobs.map((blob) => ({
src: getBlobUrl({
cid: blob.cid,
}),
})),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log(
blobs.map((blob) => ({
src: getBlobUrl({
cid: blob.cid,
}),
})),
)

Comment on lines +49 to +53
const handleKeyDown = (event) => {
if (event.key === 'Escape') {
event.stopPropagation()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to useCallback here. Otherwise the document.addEventListener('keydown', handleKeyDown) and document.removeEventListener('keydown', handleKeyDown) might not refer to the same function (resulting in the listener staying there forever)

Copy link
Contributor Author

@foysalit foysalit Dec 5, 2024

Choose a reason for hiding this comment

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

ermm.. not sure if that's correct? what would the deps for the hook be? also, wouldn't it depend on how the exit and entered handler is defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

happy to address this later and if we find this to be an issue, this will need to be updated in a few other places where lightbox is used.

@foysalit foysalit changed the base branch from reverse-takedown-in-workspace to main December 5, 2024 12:00
@arcalinea arcalinea temporarily deployed to hide-bloblist-by-default - ozone-staging PR #252 December 5, 2024 12:08 — with Render Destroyed
@foysalit foysalit merged commit c3cf1b2 into main Dec 5, 2024
3 checks passed
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.

3 participants