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

Popup fix #2657

Merged
merged 2 commits into from
Jan 21, 2025
Merged

Popup fix #2657

merged 2 commits into from
Jan 21, 2025

Conversation

raffazizzi
Copy link
Contributor

Removed links back to text in footnote and bibliography popups. Also removed dependency on jQuery for this script.

This should fix #2656

…removed dependency on jQuery for this script.
@sydb
Copy link
Member

sydb commented Jan 17, 2025

I am unable to review this well because I do not really speak Javascript. But I generated the Guidelines in this branch, put them on a server,¹ and took a look. Indeed, the problem described in the issue is resolved. Seems to accentuate the problem of the pop-up appearing quite a distance away from the link, but that is not (I don’t think) a problem for this ticket.
The result here is, I think, far better than what you get from dev, so I am in favor of merging this ASAR.

In case you want to see a comparison without generating yourself:

Note
¹ Because you never see the pop-ups if you just open the local file.

@martindholmes
Copy link
Contributor

Agreed. The fix works nicely, but we should raise another ticket for the popup location. I think this was all written back when screens were small and nothing was very far away from anything else. Easy to locate it to the left or right of the click event location, depending on the space available.

@raffazizzi
Copy link
Contributor Author

@martindholmes and @sydb, I made a few adjustments to the JS to make it more idiomatic and I improved the positioning of the popups.

More importantly, I think I removed the dependency on bootstrap and jquery. I couldn't locate any place in the code that seems to use them. I could use some help double checking that I didn't break anything.

I think the Stylesheets still use jQuery somewhere but also load their own code. JQuery is no longer necessary in 2025; as browser native APIs have caught up and are generally faster and better. It would be good to get rid of that dependency entirely, eventually.

@martindholmes
Copy link
Contributor

@raffazizzi @sydb When I wrote that popup thing, it definitely had no dependency on Bootstrap (which I don't think existed at the time) or JQuery (which I've always hated). I think Sebastian put JQuery in there from before that because it was used for some listing or table-of-contents functionality, but anything it would have been doing back then would be trivial to do in CSS nowadays.

@joeytakeda and I were talking today about the need to make the Guidelines more phone-friendly, especially those tagdocs tables at the end; in some work on the staticSearch documentation (which is generated from the ODD file with the Stylesheets) I've been working on that, and it is doable.

Copy link
Member

@ebeshero ebeshero left a comment

Choose a reason for hiding this comment

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

This looks great! @sydb and I reviewed it this evening and determined that we should probably reconsider the position of the popup, but that we can save for a later ticket!

@ebeshero ebeshero merged commit b0a5fd5 into dev Jan 21, 2025
2 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.

Footnote and bibliography popups show back to text links
4 participants