[fix] News Site Nuxt: rename 'className' -> 'class'#400
[fix] News Site Nuxt: rename 'className' -> 'class'#400flashdesignory merged 2 commits intoWebKit:mainfrom
Conversation
flashdesignory
changed the title
|
It's good to fix this for the next version but I don't think we should merge this back into 3.0 at this point. |
yeah, no need to merge into 3.0. |
| <input :id="navbarStyles['navbar-toggle']" type="checkbox" :checked="isOpen" @change="handleChange" /> | ||
| <label :for="navbarStyles['navbar-toggle']" :class="navbarStyles['navbar-label']"> | ||
| <span className="visually-hidden">Navbar Toggle</span> | ||
| <span class="visually-hidden">Navbar Toggle</span> |
There was a problem hiding this comment.
Was that used somewhere? It's not clear to me
There was a problem hiding this comment.
Nope, it would only be seen if it's rendered on mobile view, but Speedometer uses a larger viewport.
This is just being a "good citizen" and fixing stuff 😄
There was a problem hiding this comment.
TBH I think this changes nothing. I looked at the dist in https://browserbench.org/Speedometer3.0/resources/newssite/news-nuxt/dist/ and I found we properly have class attributes:
(I used Firefox' responsive mode)
It looks like Vue accepts className just as well as class.
julienw
left a comment
julienw
left a comment
There was a problem hiding this comment.
I'm surprised that for a patch that should change nothing there are so many things changed in the rendered files. Can you check why this happens?
Also it looks like some dist files were removed by mistake...
| <input :id="navbarStyles['navbar-toggle']" type="checkbox" :checked="isOpen" @change="handleChange" /> | ||
| <label :for="navbarStyles['navbar-toggle']" :class="navbarStyles['navbar-label']"> | ||
| <span className="visually-hidden">Navbar Toggle</span> | ||
| <span class="visually-hidden">Navbar Toggle</span> |
There was a problem hiding this comment.
TBH I think this changes nothing. I looked at the dist in https://browserbench.org/Speedometer3.0/resources/newssite/news-nuxt/dist/ and I found we properly have class attributes:
(I used Firefox' responsive mode)
It looks like Vue accepts className just as well as class.
Oh you're so right 🤦 .... |
|
@julienw - regarding |
Thanks for confirming; I don't mind doing cleanup, but your PR description was mentioning class assignments were missing, so I was curious if I missed anything here. I'm still surprised that there are so much changes in the generated files despite the change doing nothing... It would be good that we can make this more predictable eventually... |
yeah, nuxt generates a new hash that's being used in the filename... I'll look into it separately. |
It's not just the hash, if you look at the content of the files, the minified variable names are also different... that's so weird. |
3 participants
Two instances, where
classNameinstead ofclasswas used.This happened in areas that tests don't cover and shouldn't impact score.
More details:
on small screens the news site hides the navbar and displays a hamburger icon.
This hamburger icon had some missing class assignments due to the oversight.
Since we don't test mobile view, this is not an issue in the current tests.
@kara