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

Can we stop redefining base bootstrap .navbar-brand? #2096

Open
jrochkind opened this issue Apr 30, 2019 · 3 comments

Comments

@jrochkind
Copy link
Member

commented Apr 30, 2019

Bootstrap defines some classes for a navbar, including .navbar-brand

The standard Blacklight uses .navbar-brand for a logo, by default the Blacklight logo.

And Blacklight redefines .navbar-brand in some pretty major ways, changing it's positioning, size, and layout in a variety of ways.

Because it is adding styles to the base .navbar-brand, rather than a .navbar-brand.blacklight or .blacklight .navbar-brand or something like that, it's definitions apply to any .navbar-brand in the app -- at least on a page using Blacklight's CSS.

If you wanted to use bootstrap .navbar-brand on other pages in your app in other ways -- they get Blacklight's redefiniton, which makes it not really display as one expects a .navbar-brand too.

If you wanted the original Bootstrap navbar-brand, there's no great way I can figure out to get it. Because of how CSS works, you have to manually "undo" all the styles Blacklight adds, and restore them to what Bootstrap wanted (and potentially manually update when Bootstrap updates).

Is there a way Blacklight could scope it's CSS declaration to only apply to intentionally "opted in" elements, and not hijack all .navbar-brands in the app? I can think of several, but I'm not sure what the backwards compatibility implications are. If relying on a bare ".navbar-brand" selector having Blacklight's overridden styles is part of Blacklight's "API", then of course there's no way to do it backwards compatibly. I'm not sure what the "least disruptive" way to do it is, it depends on what actually existing apps are depending on.

@jrochkind

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

This does seem to be a new problem in BL 7.

I think in this commit:
1b9680c#diff-f7a9cebdea734b6bac2942747654869bR5

It's causing me lots of trouble in migrating my app from BL6. I was relying on being able to use standard Boostrap navbar-brand class, and BL 7 hijacks to insist that any .navbar-brand has special BL styling.

Previously it only applied it's special styling to #header-navbar .navbar-brand, so a .navbar-brand that was not in a (BL-specific) #header-navbar could still access default bootstrap styling. Now it can not.

I would like to send a PR reverting this, and going back to having a BL-specific class on the BL-specific navbar that should trigger BL-specific overrides to bootstrap navbar styles.

But @jcoyne, if you can remember the commit from Sep 2016 (that wasn't in a BL release until 7.0) -- can you say anything about what motivated it? And if it was intended/expected to takeover bootstrap .navbar-brand style entirely so there was no way to use original bootstrap one? Or if this was an unintended side effect? And what was intended exactly?

I guess the commit message says "This is the behavior prefered by CSSLint and it makes it easier to
override selectors downstream." -- the problem is, it's not okay, from my perspective, to completely and irreversibly override Bootstrap styles with no way to access the original styles.

@cdmo

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

What about just inlining an image?

@jcoyne

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@jrochkind I was probably just not considering the implications you pointed out. Feel free to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.