-
Notifications
You must be signed in to change notification settings - Fork 6
Frontend refactors #14
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
Conversation
chore: move to typescript chore: use refreshContent for initialization (fixes iamdarkle#3, this is fixed actually) chore: use dom based initialization check (usually doesn't cause the regression of iamdarkle#11) fix: css build cross-platform fix: slide and gallery carousel syncing remove: duplicated style imported in javascript remove: duplicated carousel event registration
It looks like the syncing is automatically handled by Fancybox itself. Let's remove it.
Hi, thanks for the time and effort on this, I'm going to review it ASAP and test it locally.
That's quite possible, did you refer to the syncing part or something else? |
Yep, forgot to remove this part while editing. |
I've been testing it, and it works great. I really like the improvements you've made. During the testing, I actually found an issue, but I'm sure it has nothing to do with your code. I checked, and it was already happening in the current version. If there are two galleries in the same post, the sync/index goes a bit crazy (#15). If you want, I can release a version with your changes now, or if you don't mind waiting a bit, I can try to fix this issue and include it as well. |
Hmm, my test post contains two galleries and two individual images and I didn't encounter this issue. Is that because my galleries contain only two images?
If you'd like to, I got no issue with it. 😃 |
Oh sorry, you were absolutely right, I wrote that right after trying to compare the multiple galleries case with the previous version, but cache played a trick on me. The error is actually in the current version, and this fixes it! I took the chance to add lazy loading, and IMO everything looks good, so I'm going to tag the new version (including your other PR). If anything comes up, don't hesitate to open an issue, and I'll try to help. Thanks! 🙂 |
This PR made some changes to the frontend, listed as follows:
refreshContent
for initialization (fixes not working when post edited #3)refreshContent
is possible (won't cause regression of Duplicated popups on image click #11 in theory)Fix Fancybox slide and gallery carousel's syncingNow it works out of box with FancyboxduplicatedFancybox's carousel event registration (it's completely useless now)I think some places can be deleted as they seem to be redundant to me, but I'm not sure of the back story that makes them there. So I left them untouched.Although I've tested it and it all seems fine and backward compatible, I would like some double-quality checks and feedback from you.