Feature - Show images in a LightBox when clicked #1473
Conversation
dosubot
bot
added
size:L
|
Simple demo of the feature: chainlit.webm |
There was a problem hiding this comment.
This is obviously a nice feature and a proper iteration on the last fixup to image display.
But we should make sure to either drop legacy behaviour entirely (and be careful not to increase our bundle size too much) or not change default behaviour and really make lightbox optional. In the latter case, the js code should only be loaded if the lightbox is enabled. My suggestion would be the latter. Regardless, I would like to see the increase in bundle size, please!
In addition, I would also like to ask for E2E testing both with and without the lightbox enabled. You can prepare the test with it disabled on a fork of main, so you can be really sure that your patch does not break pre-existing UX. Here's some suggestions of where you might want to put that: https://github.com/search?q=repo%3AChainlit%2Fchainlit+path%3A%2F%5Ecypress%5C%2Fe2e%5C%2F%2F++image&type=code
Then, add a lightbox E2E test specifically testing with the lightbox enabled.
If the original contributor needs support, I imagine there's other community members who'd like to help out getting this big UX improvement in!
Lastly a question; does this particular lightbox also work for video?
.gitignore
Outdated
| @@ -60,3 +60,5 @@ dist-ssr | |||
| .coverage | |||
|
|
|||
| backend/README.md | |||
|
|
|||
| myenv | |||
There was a problem hiding this comment.
This is specific to your dev environment and should not be in this PR!
Protip: update gitignore in your system's git config/home directory!
There was a problem hiding this comment.
😅 Agree, had to swap to my private hardware, and forgot to check that the global git config was set up.
| import Lightbox from 'yet-another-react-lightbox'; | ||
| import 'yet-another-react-lightbox/styles.css'; | ||
| import Zoom from 'yet-another-react-lightbox/plugins/zoom'; | ||
| import Download from 'yet-another-react-lightbox/plugins/download'; |
There was a problem hiding this comment.
That's a lot of dependencies to import, increasing binary size even if the feature is not turned on. How much larger is the output bundle -- and could we somehow make the lightbox optional in the frontend?
Perhaps we can set up stuff so it's a separate chunk, only importing the chunk in the browser if and when the lightbox feature is enabled?
| } | ||
| }; | ||
|
|
||
|
|
There was a problem hiding this comment.
I like how this lightbox code is much more minimal. However, if I read this well, with the lightbox disabled the old image click behaviour is entirely removed.
That's not the way forward; developers and users expect what worked before to keep working towards the future and not suddenly disappear. So let's truly make lightbox optional and make sure that we retain the exact legacy behaviour unless the lightbox is enabled.
There was a problem hiding this comment.
I agree with this, but since the original feature was not released at the time of making this, i figured that it could be removed all together. Ill fix that.
| @@ -34,4 +34,4 @@ | |||
| "micromatch@<4.0.8": ">=4.0.8" | |||
| } | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Missing trailing newline. You might want to check your text editor's config!
There was a problem hiding this comment.
Thanks for the requested changes.
There's still one missing \n -- please take care to set up your editor appropriately (this goes for all code projects).
Other than that, there's one question about the download.
I'd also like to invite community members to play with this and give feedback before merging.
| backend/README.md | ||
|
|
||
| myenv | ||
| backend/README.md |
There was a problem hiding this comment.
Trailing newline issue. https://thoughtbot.com/blog/no-newline-at-end-of-file
There was a problem hiding this comment.
Sorry for dropping the ball on this a bit, but sometimes too much is happening to do community work :(
All the work has been done on my old private laptop, so yeah, my editor was just set up quick and dirty.. Sorry about that.
Im more than happy to hand this over to someone with more time, as im quite busy two more weeks
| link.href = url; | ||
| link.download = imageName || 'image'; | ||
| link.click(); | ||
| window.URL.revokeObjectURL(url); |
There was a problem hiding this comment.
Why do we have a custom fetch() here rather than just download=true?
Download URL's do not require auth tokens, nor will it in the future (we're using cookie auth instead #1521).
Ref: https://yet-another-react-lightbox.com/plugins/download
There was a problem hiding this comment.
I had this idea of keeping plugins to a minimum, but with the lazy load you suggested, that might not be needed anymore.
And, to be honest, this is my first try on TS ever, so iv been confused.
Spent way to long trying to make E2E tests, before I had to put everything on hold.
There was a problem hiding this comment.
Pretty good for a first try. :)
I know, building these tests, especially in the beginning, is quite tedious. But once you get the hang of it, additional tests (per project), go faster. Really appreciate your work on this, thanks!
Now on this issue, you're already using the download plugin. ;)
Just putting the URL there might work. Let me know if you're gonna add that to the PR. With that, I'm happy to merge.
There was a problem hiding this comment.
Ill look into it after work tomorrow and report back. Thanks for the guidance and for all the amazing work you and your team does <3
Feature - Image LightBox
Why?
After seeing that this feature was requested back in Nov 2023, and since I couldn't find any implementation of it, I decided to implement it myself.
During my work I discovered #1402 which is a PR that implements a simple popup for images. While this is an improvement over the current situation, I believe that a lightbox is a better solution for this problem.
Added dependencies
I chose this libary due to its MIT license and that it is actively maintained.
Disclaimer
Im not a frontend developer, so I might have missed some best practices or made some mistakes. Please let me know if you see anything that could be improved.