Upgrade bootstrap and fix scrollspy (Lombiq Technologies: NEST-594) #18019
Conversation
| @@ -1,11 +1,11 @@ | |||
| /*! | |||
| * Start Bootstrap - Agency v7.0.11 (https://startbootstrap.com/theme/agency) | |||
| * Copyright 2013-2022 Start Bootstrap | |||
There was a problem hiding this comment.
I think such changes need to be done on the theme repo itself
There was a problem hiding this comment.
Is there a separate repository?
Btw this is a generated change.
There was a problem hiding this comment.
I'm not sure what you're referring to, but we should add fixes to the original Bootstrap template, if viable (not always, we've waited years for PRs to be merged before). However, these changes come from 5.3.6 of the Bootstrap template.
There was a problem hiding this comment.
These changes need to be addressed here, as well as in the theme repository. I fixed problems like this; this is basically the theme repo manually transformed to an OC theme.
If it's not viable (since we need to wait a lot to get the PR merged), then at least create an issue in the repository.
However, these changes come from 5.3.6 of the Bootstrap template.
Then it's basically just updating the OC theme to match with the Bootstrap one, so no PR is needed there, if I'm correct.
|
Updated to the newest files of the template itself. Then ran Not sure what to do with the Validating the Building of Public Assets workflow, tried to add the popperjs in the dependant version, but that did not work. Is there documentation to troubleshoot this? |
|
Start from here: https://docs.orchardcore.net/en/latest/guides/assets-manager/#prerequisites And run the command not in the wwwroot folder but in the repo root. |
Yeah, I did exactly that. I get the same "error" as the GitHub workflow: After these, running |
|
I committed the changed |
|
I can only recommend you to look into the error message. That, and what comes from CI, suggests an improper NPM installation, what might be possible to solve simply with an |
The error message was not helpful at all. 🥲 It had nothing to do with why the GitHub workflow failed. My solution: |
| "chokidar": "3.5.3", | ||
| "concurrently": "6.3.0", | ||
| "postcss": "8.4.21", | ||
| "prettier": "2.8.6", | ||
| "pug": "3.0.2", | ||
| "sass": "1.60.0", |
There was a problem hiding this comment.
Why downgrade these dependencies?
There was a problem hiding this comment.
There was a problem hiding this comment.
As these are dev dependencies I think it is fine to update them, especially as it was working before with the newer versions.
There was a problem hiding this comment.
It's fine by me. Others asked me to use the newest template instead of a custom fix. So I used the template.
Let me know what you think @sebastienros if we should use the template entirely or keep these dev dependencies like they were before.
There was a problem hiding this comment.
I'd rather see we do these changes on the source repository and then sync back.
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
@wAsnk feel free to merge when you think it's ready |
# Conflicts: # yarn.lock
|
@wAsnk could you please fix the asset build? I wanted to merge this, seeing it was approved, and tried to fix the merge conflict, but I'm now out of ideas. |
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
|
I'll check it now. Sorry about the delayed response. |
# Conflicts: # yarn.lock
|
Thanks! |
|
@Piedone I think this is ready to be merged. |
|
Thank you! |
Fixes #18018