-
-
Notifications
You must be signed in to change notification settings - Fork 71
Account for text fragments when autoexpanding post history items #1928
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
trichoplax
left a comment
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.
Great to see more jQuery being retired.
The functionality of each previous jQuery line is replicated without the vulnerability to text fragment directives in the URL, so this looks complete to me, and I've approved.
The only comments I've made are a matter of personal preference where I can't guess what will be easier to read for different people, so I've just listed some thoughts. I'm fine with this being merged as is though.
cellio
left a comment
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.
I tested on my dev server with the URL http://localhost:3000/posts/46/history#2:~: and it went to the correct history entry. I don't know what:~: is supposed to do (I got that from the bug report), but it doesn't do something obviously bad now. If @trichoplax (who understands the problem more than I do) is happy, I'm happy.
As a quick FYI, |
closes #1925