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

Remove jquery.inview JavaScript library #1803

Merged
merged 3 commits into from
Dec 8, 2024

Conversation

adamzap
Copy link
Member

@adamzap adamzap commented Dec 7, 2024

The jquery.inview library was being used to trigger an animation when an icon in a feature list element is fully visible on screen. This patch replaces jquery.inview with an implementation based on IntersectionObserver, which is widely supported: https://caniuse.com/intersectionobserver.

This patch also simplifies the structure of list-feature.js.

The `jquery.inview` library was being used to trigger an animation when
an icon in a feature list element is fully visible on screen. This patch
replaces `jquery.inview` with an implementation based on
`IntersectionObserver`, which is widely supported:
https://caniuse.com/intersectionobserver.

This patch also simplifies the structure of `list-feature.js`.
Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know if anything should change here style-wise.

Copy link
Member

Choose a reason for hiding this comment

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

No comment on the style, other than I think we should use an autoformatter so we don't have to think about it (I thought the project had prettier configured in pre-commit, but maybe it's not run on js files? 🤔 )

That's out of scope for this PR though.

@adamzap
Copy link
Member Author

adamzap commented Dec 7, 2024

To verify that this works, you'll want to check that the icons on the homepage and the overview page animate as before:

image image

Copy link
Member

@bmispelon bmispelon left a comment

Choose a reason for hiding this comment

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

Another really nice cleanup 🚀

It's pretty amazing to see how far the web has come in the 10 years since this design was made. All these jquery plugins are now either not needed or can be replaced by native APIs.

I just had two very minor comments. The functionality is indeed working. 👍🏻


// Export a single instance of our module:
return new FeatureList('.list-features');
}, { threshold: 1.0 });
Copy link
Member

Choose a reason for hiding this comment

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

A comment similar to the previous // element completely visible would be welcome here (had to look up the specs to understand what threshold meant)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7b85d55.

return new FeatureList('.list-features');
}, { threshold: 1.0 });

$('.list-features').find('i').each(function () {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this equivalent to:

Suggested change
$('.list-features').find('i').each(function () {
$('.list-features i').each(function () {

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in b24f10b.

Copy link
Member

Choose a reason for hiding this comment

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

No comment on the style, other than I think we should use an autoformatter so we don't have to think about it (I thought the project had prettier configured in pre-commit, but maybe it's not run on js files? 🤔 )

That's out of scope for this PR though.

Copy link
Member

@bmispelon bmispelon left a comment

Choose a reason for hiding this comment

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

Excellent 👍🏻

@bmispelon bmispelon merged commit 5cad7d2 into django:main Dec 8, 2024
4 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.

2 participants