-
Notifications
You must be signed in to change notification settings - Fork 359
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
Add class for components #4071
Add class for components #4071
Conversation
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.
Thanks, @LuomaJuha -- see below for a few questions, comments and suggestions.
module/VuFind/tests/unit-tests/src/VuFindTest/View/Helper/Root/ComponentTest.php
Outdated
Show resolved
Hide resolved
themes/bootstrap5/templates/_ui/components/hide-offcanvas-button.phtml
Outdated
Show resolved
Hide resolved
themes/bootstrap5/templates/_ui/components/show-sidebar-button.phtml
Outdated
Show resolved
Hide resolved
…/ComponentTest.php Co-authored-by: Demian Katz <demian.katz@villanova.edu>
…ha/NDL-VuFind2 into add-wrapper-class-option
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.
Thanks for addressing all of my comments, @LuomaJuha -- just a couple more thoughts now...
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.
Now that I gave this another thought I'd like a bit of explanation why this is needed and a good thing. I'm not sure the possible benefits outweigh the potential confusion this can cause if a theme's styles target another.
Reverted the changes for theme-specificity, as it could have caused confusion, but left the _componentClass to be used later for overwriting styles |
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.
Thanks for the simplification, @LuomaJuha! Just one last documentation-oriented suggestion below.
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.
Thanks, @LuomaJuha -- looks good to me, and all tests are still passing.
I can't merge this without dismissing @EreMaijala's open review, so I'll wait and see if he has any final thoughts before finalizing the work.
Sure! |
Adds a class for components, so adjusting styles locally is easier. Small adjustments to the code.