-
-
Notifications
You must be signed in to change notification settings - Fork 933
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
Refactor stimulus nav controllers to follow stimulus best practices #4422
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4422 +/- ##
=======================================
Coverage 97.13% 97.13%
=======================================
Files 385 385
Lines 8200 8200
=======================================
Hits 7965 7965
Misses 235 235 ☔ View full report in Codecov by Sentry. |
b3df888
to
b2997aa
Compare
171ead7
to
896c06b
Compare
896c06b
to
dffcfed
Compare
I made a major refactor of these now that @colby-swandale got me started on learning more about stimulus. The header search box has some very weird behavior. I hid it on the home page, but it doesn't always solve the problem. Sometimes you can tab to it and then it stays visible, so I started hiding it purposely when the sandwich is clicked to close. It's better than it was, but you can still get in a weird state depending on the browser behavior for tabbing. |
@@ -185,25 +185,25 @@ | |||
border-radius: 22px; } | |||
|
|||
@media (min-width: 1020px) { | |||
.header__popup__nav-links, .home__header__popup__nav-links { |
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.
The CSS changes are flipping the logic from is-expanded
to hidden
to be in line with the stimulus-dropdown component (which I'm not using, but still conformed to for simplicity).
dd6b79c
to
9d119c8
Compare
9d119c8
to
cb401be
Compare
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.
Looks good to me, but I'm total stimulus noob. Better to wait for @colby-swandale review. 🙏
cb401be
to
16ea06d
Compare
Remove unused javascript from nav.
16ea06d
to
e1dec5f
Compare
Hey, I'm going to merge this so I can use it as a base for autocomplete and fix the console errors. If there's any feedback, please let me know and I'll update. |
Change nav stimulus controllers to be more true to stimulus best practices (and be much simpler).
Also fixes the original error that triggered this PR.