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

Sync with upstream (Jan 9, 2025) #319

Merged
merged 4 commits into from
Jan 13, 2025
Merged

Sync with upstream (Jan 9, 2025) #319

merged 4 commits into from
Jan 13, 2025

Conversation

lydiascarf
Copy link
Collaborator

Okay folks, so we have gotten ourselves into a bit of a mess with diverging histories from upstream. This is made a bit more challenging because we're no longer technically a fork of JKAN on GitHub, so we can't use any of GitHub's tooling to sync up.

I'll leave the branch up until this PR is merged so you can see for yourself, but for posterity, this is what the result of a rebase looked like when i tried in July (sorry for taking so long to get back to this 😅):

Screenshot 2025-01-09 at 8 18 39 AM

Many of the new "changes" seem to just be files and lines we already have, so not sure what happened there.

Then i tried merging and resolving the conflicts, but this felt like an error prone process, and confusingly that PR wouldn't merge unless i squashed all of the commits. The whole PR was starting to feel like no matter what it would make our history much messier. See here.

So this morning i decided to just go through and cherry-pick the specific commits from upstream and resolve a couple issues that came up in the process. I'm going to make a separate upstream PR when i have time to make sure that jkan isn't missing any important improvements from our end, but that's the excruciatingly long story behind this little set of changes.

BryanQuigley and others added 4 commits January 9, 2025 07:33
…uilt-in to debian bookwarm

It's nodejs 18.13.0
Some of the attributes changed with bootstrap 5. Also removed the redundant screen reader label (it's already present in the aria-label attribute and was showing on screens)
The GitHub link was placed in a way that interacted weirdly
with the nav bar toggle and used an outdated href. Also, the
navbar was using a light theme, so it didn't show up well.
Resolves an issue with rexml dependency during bundling
@lydiascarf lydiascarf self-assigned this Jan 9, 2025
@lydiascarf lydiascarf linked an issue Jan 9, 2025 that may be closed by this pull request
Copy link
Member

@BryanQuigley BryanQuigley left a comment

Choose a reason for hiding this comment

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

I don't see where bundle exec came from, but +1

It does seem like the rebase process doesn't really scale right now :(

@lydiascarf
Copy link
Collaborator Author

@BryanQuigley maybe bundle exec was just necessary on my machine. feel free to try running on your local without it. i think in this case it's just protecting us from using the wrong version of a dependency that i have elsewhere in my environment for some reason (?). not sure exactly but can look more closely later.

@BryanQuigley
Copy link
Member

You are correct, I wonder when thatbroke

@BryanQuigley BryanQuigley merged commit df6cddb into main Jan 13, 2025
3 checks passed
@BryanQuigley BryanQuigley deleted the 20250109-upstream-sync branch January 13, 2025 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix or find replacement for navbar-toggler
3 participants