Skip to content
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

fix: don't override toggleAttrs in menu-button. #3381

Merged
merged 4 commits into from Feb 27, 2024

Conversation

crhallberg
Copy link
Contributor

Thanks for the spot on this, @demiankatz

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @crhallberg. Unfortunately, it looks like adding the btn class to the control has some side effects.

In every theme, it changes the mouseover highlight box from being fully rectangular to having rounded corners. Since the control is currently used in the header, this causes some inconsistency, since the highlight on the "login" control looks different from the highlight on the language/theme controls immediately next to it. Maybe this is justifiable, but it's a little weird. Perhaps the solution is to add the btn class to the login control for consistency.

More serious is the effect in the bootprint3 theme. Here's what it looks like in dev:

image

And here's what it looks like in this branch:

image

Any thoughts/suggestions/preferences?

@crhallberg
Copy link
Contributor Author

I'm not opposed to removing btn. It's not essential for function and can be easily overwritten where we need it. I can't remember why we added btn though. Which buttons did it benefit?

@demiankatz
Copy link
Member

I'm not opposed to removing btn. It's not essential for function and can be easily overwritten where we need it. I can't remember why we added btn though. Which buttons did it benefit?

If I'm remembering correctly, we needed the btn class to eliminate an unwanted outline in findingaugustine.org's language drop-down control. But if there's no benefit to the btn class in the core project, we can easily add it back in that instance as a customization instead of making the change globally.

@crhallberg
Copy link
Contributor Author

crhallberg commented Feb 27, 2024

we can easily add it back in that instance as a customization instead of making the change globally.

I think that's the most widely compatible option. .btn removed in 1ccd21f.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @crhallberg!

@demiankatz demiankatz added this to the 10.0 milestone Feb 27, 2024
@demiankatz demiankatz merged commit 414ef80 into vufind-org:dev Feb 27, 2024
7 checks passed
@demiankatz demiankatz deleted the menu-button-attrs branch February 27, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants