-
Notifications
You must be signed in to change notification settings - Fork 56
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
Upgrade to Blacklight 8 and Bootstrap 5 #1441
Conversation
654eca4
to
083a3ec
Compare
I'm working on 1 & 2. |
8189ef4
to
23aba61
Compare
Demo app download link: https://github.com/geoblacklight/geoblacklight/actions/runs/10255680191/artifacts/1778135355
|
23aba61
to
5f93bf3
Compare
@thatbudakguy looks great. Builds correctly. Thank you for all your work on this. |
The homepage jumbotron uses the same background color as the search bar. I thought it looked OK, so I left it 🤷🏻 |
<%= search_bar %> | ||
<% else %> | ||
<div class='geobl-homepage-masthead jumbotron'> | ||
<header> |
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.
Should we do this in Blacklight too? https://github.com/projectblacklight/blacklight/blob/main/app/components/blacklight/header_component.html.erb
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.
In Argo, we didn't put the search_bar in the <header>
, it went in top_bar instead. I wonder if we can find consistency on that. https://github.com/sul-dlss/argo/blob/main/app/components/top_navbar_component.html.erb#L1
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.
I'm happy to adopt whatever Blacklight does 🤷🏻
This removes the GeoblacklightHelperBehavior that was created to generate our search result preview descriptions and makes it into a tested component method instead, so that there's less overriding of Blacklight.
5f93bf3
to
4600611
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.
- Points 1,2 are much better.
- I hadn't noticed the deafault logo issues before. It's not a huge deal since it's almost always replaced. The logo is an svg. I wonder if we can update the color. If not in this branch in another ticket.
- In the newest version of this branch, clicking on a feature in an index map does not update anything on the page and raises an error in the console. I haven't dug into it too far:
Uncaught (in promise)
TypeError: Cannot set properties of null (setting 'innerHTML')
4600611
to
ef35ad7
Compare
Good catch! the new |
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.
This looks good now. I'm going to approve to get it moving, but I opened a follow-up Issue that should be resolved before cutting a release: #1464
Now that geoblacklight/geoblacklight#1441 has been merged we should probably be using main?
Now that geoblacklight/geoblacklight#1441 has been merged we should probably be using main?
Now that geoblacklight/geoblacklight#1441 has been merged we should probably be using main?
Now that geoblacklight/geoblacklight#1441 has been merged we should probably be using main?
This supersedes the earlier PR from which it was inspired, #1344.
Closes #1373 and #1292.