oppia: [BUG]: Remove unused CSS classes
Describe the bug
We have some unused classes defined in our CSS files, for example, .conversation-skin-future-tutor-card is defined in /core/templates/pages/exploration-player-page/learner-experience/tutor-card.component.css but appears nowhere else.
Write a script (perhaps in the form of a linter) to identify all such unused classes; then open a PR to remove them. To make sure these unused classes are removed in the future, consider including a linter in your PR.
URL of the page where the issue is observed.
No response
Steps To Reproduce
No response
Expected Behavior
No response
Screenshots/Videos
No response
What device are you using?
No response
Operating System
No response
What browsers are you seeing the problem on?
No response
Browser version
No response
Additional context
No response
Tips for developers
Before tackling the bug, please use git bisect (see https://git-scm.com/docs/git-bisect) to investigate which PR caused it (you only need to go back as far as commit https://github.com/oppia/oppia/commits/9a334e9). If you find the PR, leave a comment on this issue pointing to it, or just say that it happened before commit 9a334e9 if you could reproduce it there. This will help us fix the issue by reverting the faulty PR.
Also, if this is your first issue, please make sure to follow https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#choosing-a-good-first-issue and https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#setting-things-up before claiming it. Thanks!
About this issue
- Original URL
- State: open
- Created 9 months ago
- Comments: 40 (13 by maintainers)
(Also, I’m going to assign you this issue since I think you have a great handle on it at this point 😃 )
That’s great, that looks really nice @wangjess. Thanks a lot!
The only loophole I can see to this is if CSS classes are ever generated or applied programmatically (i.e. through TypeScript). So maybe take a look at the specific element the CSS class was on before and after the change, and search for some fragment of it to verify that it’s not being used in a TS file. (I don’t think there should be such cases, and we really should get rid of all such usages – but this is just me trying to think about being thorough.)
The only other thing I noticed is that all the files in the to_delete/ category (I checked 10ish of them manually) all seem to have exactly 1 item. Might be worth checking that, if that’s true for all the files, that it isn’t a coincidence (e.g. it’s not just taking the first class in the list for some reason).
But overall this looks really great, thanks again!
@seanlip Update: I will be out of town for Oct 11 - 13 and I can’t work on this ticket during then.
Right now tinycss2 appears to get all of the class names - I grabbed a few class names from various file(s) and verified their existence in the findings text files, and everything looks good. Also
conversation-skin-future-tutor-cardis getting found too, which is good (the example class provided in this ticket description).All right. In that case, if both of you come up with different approaches that work, we can examine both and decide together on the best one for long-term maintainability. Otherwise, we’ll go with the first feasible approach for which proof (ie the list of necessary fixes) is provided.
@wangjess Ah – great question! Please ignore files in third_party/, node_modules/, dist/, scripts/ and .direnv/. Those are either test files, libraries we don’t control, or files that are generated, and are not relevant for this issue.
Everything in /core/ or /extensions/ is fair game, though. I hope that helps!
Hi @wangjess, the files in /webpack_bundles are autogenerated. You can skip them. Thanks for checking!
I am working on this ticket following the rough guideline provided by you @seanlip.
My approach: