-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
Backport popover code from Kiwix PWA #1252
Conversation
Nice one @audiodude! For now, I'm still working through things and fixing stuff, but I'll be very pleased to request a review when it's ready! Still some rough stuff... |
@audiodude All done, should be ready now for you to move on to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment, about file structure/code location. I don't want to review more until you resolve because it would change where my comments go and I would get the stupid github "outdated" nonsense.
New test deployment as v4.0.9 here:
Ensure it has updated to v4.0.9 (message in Configuration should appear after 15s) and restart or reset if necessary. In Firefox a Reset (Expert Settings) is often needed due to few-hours limit on Service Worker checking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. I can see what you mean about the pain of having to deal with Promise and callbacks in the same code base, ick. I've made lots of suggestions about refactoring and extracting local helper functions, I think it will really help readability especially in the aforementioned context.
@audiodude Many thanks for helpful comments once again. It's getting towards Friday evening here, so I'll try to resolve everything this weekend. Have a great weekend when you get there! 😊🍾 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in the next push to the PR, I'm refactoring the code to address the identified issues. Some explanatory comments below. I'll close conversations before pushing, or else it gets hard to close them even using the GitHub UI. Many thanks, I think the code is more accessible now, and unneeded (if harmless) Promise resolving should have been fixed.
New test deployment as v4.0.91 here:
Ensure it has updated to v4.0.91 (message in Configuration should appear after 15s) and restart or reset if necessary. In Firefox a Reset (Expert Settings) is often needed due to few-hours limit on Service Worker checking. |
@audiodude Are you happy with the resolved issues? OK to merge after final testing? NB future PRs will certainly not be as massive as this! This was the outcome of work at the Hackathon, hence a major feature. It's rare to do major features! At some point there will be a PR (see #1253) to add these popovers to new tabs / windows controlled by the app's Service Worker, but not for this PR which is feature-complete as far as I can manage for now. Adding to new tabs or windows needs careful thought, as we have to keep track of opened windows, which we currently don't do explicitly (just let the SW do that job, but it knows nothing about the DOM). |
Sorry for the delay. LGTM. |
Many thanks for your help @audiodude -- you really helped improve the code! 🙏 |
Last manual tests:
Good to merge. |
Fixes #719. This backports the new feature from the PWA.
So far, I have ported the mouseover and the focus events, together with the popover code. Focus event provides keyboard support (tabbing into a link).
To do:
Add popover support to new windowsOpen a new issue to add popover support to new tabs/windows: Extend popover support to new tabs or windows #1253