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!
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.