Skip to content

fix: remove stylelint-config-twbs-bootstrap and pin dep version#37

Merged
darren-arbon merged 6 commits intomainfrom
TA-470-fix-dependencies
Feb 21, 2024
Merged

fix: remove stylelint-config-twbs-bootstrap and pin dep version#37
darren-arbon merged 6 commits intomainfrom
TA-470-fix-dependencies

Conversation

@twba-a
Copy link
Copy Markdown
Contributor

@twba-a twba-a commented Feb 20, 2024

Problem:

TA-470
Our bootstrap theme project is in a bit of a mess, if you checkout the project and run npm i you’ll be met with a load of peer dependency errors, this has likely come from us not pinning the dependencies to specific versions.
A lot of the warnings revolve around stylelint-config-twbs-bootstrap which provides a stylelint config, although we don’t actually follow the rules set by this config and it doesn’t match the ruleset used in other projects, so we should probably just remove it.

investigation result:

  • No changes would happen on css assets if SASS is on version 1.55.0. Updating to version 1.62.0, as specified in the lock file, alters custom-full css assets but does not result in any noticeable visual changes on the pages.

  • Upgrading Bootstrap to version 5.3 will impact the colours of the alerts and may affect other styling aspects.

  • Section for horizontal forms in the index.html has been added, which is not reflected on our live site at https://talis.github.io/bootstrap-theme. Findings about that:

    The section should be present since it's included in forms-2.njk Nunjucks file . This file is then included as a template in the index.njk, and subsequently generated into the index.html file using @11ty/eleventy.
    However, it seems that @11ty/eleventy was failing to generate that section, possibly due to version inconsistencies with other dependencies.

Screenshot 2024-02-21 at 07 47 05

Fix:

  • removed stylelint-config-twbs-bootstrap
  • pinned the versions according to the lock file, which updates custom-full css assets but doesn't visually affect the site. Also, it generates the horizontal form in the index.html, which is desirable as it exists in the forms Nunjucks template.

To test:

  • run npm ci
  • run npm run start

@twba-a
Copy link
Copy Markdown
Contributor Author

twba-a commented Feb 20, 2024

I was not sure wether to look for upgrades or not. for example bootstrap latest is 5.3 . Let me know if I should look into these as part of this

@darren-arbon
Copy link
Copy Markdown
Contributor

Whens me does run the npm build then it makes changes to some of the css files. It says that custom.css has changed, but I could not work out where. But, custom-full.css has a few little changes in it.....

@twba-a
Copy link
Copy Markdown
Contributor Author

twba-a commented Feb 20, 2024

Whens me does run the npm build then it makes changes to some of the css files. It says that custom.css has changed, but I could not work out where. But, custom-full.css has a few little changes in it.....

right! I see the files now. I was only comparing the user interface ><

Comment thread docs/assets/css/custom-full.css Outdated
@@ -1,6 +1,6 @@
/*!
* Bootstrap v5.2.2 (https://getbootstrap.com/)
* Bootstrap v5.2.1 (https://getbootstrap.com/)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We're downgrading bootstrap?

Copy link
Copy Markdown
Contributor

@junglebarry junglebarry Feb 20, 2024

Choose a reason for hiding this comment

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

npm --package-lock-only ls

will tell you what is currently installed from the package lock.

I don't really understand the benefit of pinning - if we use ci it uses the package lock anyway, which is always pinned. However, if we are going to pin, it should be to what is currently used.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@talis/bootstrap-theme@3.0.7 /Users/david/talis/bootstrap-theme
├── @11ty/eleventy@2.0.1
├── @commitlint/cli@17.6.1
├── @commitlint/config-conventional@17.6.1
├── autoprefixer@10.4.14
├── husky@8.0.3
├── lint-staged@13.2.1
├── nodemon@2.0.22
├── npm-run-all@4.1.5
├── postcss-cli@10.1.0
├── postcss@8.4.23
├── prettier@2.8.8
├── purgecss@5.0.0
├── sass@1.62.0
├── serve@14.2.0
├── standard-version@9.5.0
├── stylelint-config-twbs-bootstrap@7.0.0
└── stylelint@15.6.0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree with you on that we should use ci, but developers may opt for i. Pinning them would ensure that we avoid encountering a situation similar to our current one, where the lock file contains higher versions than those specified in the json.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

let me know if you still think we should not pin them. I have no issue reverting that, but will need to make sure to update the README file at least as it says use npm i

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've approved the changes.

Might be worth following up with enforcing save-exact to cover future packages.

@twba-a
Copy link
Copy Markdown
Contributor Author

twba-a commented Feb 21, 2024

This is ready for review . I've outlined the findings and observations in the PR description for your review. Thanks

Comment thread docs/index.html
<script src="https://kit.fontawesome.com/1f723026cf.js" crossorigin="anonymous"></script>
<script src="https://cdn.jsdelivr.net/npm/bootstrap@5.0.1/dist/js/bootstrap.bundle.min.js" integrity="sha384-gtEjrD/SeCtmISkJkNUaaKMoLD0//ElJ19smozuHV6z3Iehds+3Ulb9Bn9Plx0x4" crossorigin="anonymous"></script>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@twba-a did you manually change this file and re-save it? Wondering if these are caused by our touching the files or the build process...

Copy link
Copy Markdown
Contributor Author

@twba-a twba-a Feb 21, 2024

Choose a reason for hiding this comment

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

sorry if I wasn't clear in the description but this file is not touched. This file gets generated by @11ty/eleventy
its generated from nunjack index file index.njk .

Copy link
Copy Markdown
Contributor

@junglebarry junglebarry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for humouring me on pinning versions.

@darren-arbon darren-arbon merged commit 777233c into main Feb 21, 2024
@darren-arbon darren-arbon deleted the TA-470-fix-dependencies branch February 21, 2024 10:15
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.

3 participants