Added lazy loading attribute to thumbnail image tags

m3tal10 commented 1 week ago

Summary

I added a lazy loading attribute to thumbnail image tags to load images outside the viewport lazily. Initially, I checked the browser devtools to check if the <img> tag contains the loading='lazy' attribute. Then I checked the network tab to confirm whether the network fetches the thumbnail images lazily. Also fixed a simple issue where the image tag was not self-closing. This might cause HTML issues. So, I thought this would be a good idea to self-close the image tags.

References

Issue: #8266

Reviewer guidance

The reviewer can test via the browser devtools. They can inspect the thumbnail image and check if the loading attribute is set to lazy. …

Testing checklist

[x] Contributor has fully tested the PR manually
[ ] If there are any front-end changes, before/after screenshots are included
[ ] Critical user journeys are covered by Gherkin stories
[ ] Critical and brittle code paths are covered by unit tests

PR process

[x] PR has the correct target branch and milestone
[ ] PR has 'needs review' or 'work-in-progress' label
[ ] If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
[ ] If this is an important user-facing change, PR or related issue has a 'changelog' label
[ ] If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

MisRob commented 1 week ago

Welcome @m3tal10 and thanks! I will let @rtibbles to confirm as you two chatted together, but so far looks like intended behavior in these components where you've started work.

m3tal10 commented 1 week ago

Hello @MisRob , That's great to hear that it works as expected but should I also do it somewhere else or this is just fine? Like if there's still something left I should start working if this ain't enough, Like I saw that my name should be placed in AUTHORS.md. Should I do it myself or a reviewer should do this?

github-actions[bot] commented 1 week ago

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.0.dev0_git.20241116110418.pex
Windows Installer (EXE)	kolibri-0.18.0.dev0+git.20241116110418-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.0.dev0+git.20241116110418-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.0.dev0+git.20241116110418.dmg
Android Package (APK)	kolibri-0.18.0.dev0+git.20241116110418-0.1.4-debug.apk
TAR file	kolibri-0.18.0.dev0+git.20241116110418.tar.gz
WHL file	kolibri-0.18.0.dev0+git.20241116110418-py2.py3-none-any.whl

rtibbles commented 6 days ago

For reasons I can't quite discern, the automated linting check here has passed, however when I run it locally, it definitely fails.

I would recommend rebasing this PR on latest develop also, as we have merged an important change since yesterday.

m3tal10 commented 5 days ago

@rtibbles , Void tags in HTML are indeed self-closing. here is a screenshot taken from mdn docs. link: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img All the void tags are self-closing in the examples. From the source you have provided, explicitly closing a void tag is XHTML compatible. I think this ensures universal access from different browsers and devices that are XHTML compliant. As this practice is also compliant with HTML, I think following XML standards is more future-proof. What is your opinion about this? Like, using <img/> like this does not break HTML codes and it is also compliant with XML standards but <img> like this breaks XML standards and generally throws a parsing error in XML parsers. I am a beginner and I want to know why we are so strict about not using self-closing tags in HTML. I think learning this will fulfill my curiosity.

The thing that I'm worried about is, why pre-commit hook linting is not working on my system also it passed the GitHub actions linting checks. I will try to fix it. If you have any guidance, I will be happy to get some.

m3tal10 commented 5 days ago

I think I have finished adding lazy loading attribute to <img> tags. Can you check now @rtibbles ?

rtibbles commented 5 days ago

I am hoping it was just a weird one off with the state of the develop branch when you happened to make your feature branch - the linting all looks correct now though.

m3tal10 commented 5 days ago

Yeah because I turned off my code editor's auto formatting and fixed the errors manually. The linting shows error in the code editor but it does not throw error in the pre commit/workflow phase. The pre-commit hook does not work for me, I can tell it for sure. If I make formatting errors and then try running the pre-commit hook, it passes the test even if I do not follow the formatting rules. @rtibbles

rtibbles commented 5 days ago

Hrm, that's somewhat troubling - I wasn't able to replicate that when I pulled your code in locally. I wonder if a later version of pre-commit might be the difference here.

m3tal10 commented 5 days ago

I guess so. Btw, can you review my PR and check if I should do something else or is this okay?

MisRob commented 2 days ago

Congrats to your first contribution @m3tal10 !

learningequality / kolibri