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

Drop Bootstrap and Font Awesome git submodules #889

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

chalin
Copy link
Collaborator

@chalin chalin commented Feb 14, 2022

  • Contributes to Eliminate Bootstrap and Font Awesome git submodules #948
  • There are not changes to the generated site files except for the (non-production-only) User Guide scss/main.css.map file, as can be expected -- because the path to various resources has changed, e.g.:
    diff --git a/scss/main.css.map b/scss/main.css.map
    index b4c1819..5ebbaf1 100644
    --- a/scss/main.css.map
    +++ b/scss/main.css.map
    @@ -8,111 +8,111 @@
      	  "assets/scss/_variables_project.scss",
      	  "../assets/scss/_variables.scss",
      	  "../assets/scss/support/_mixins.scss",
    -		"../assets/vendor/bootstrap/scss/bootstrap.scss",
    -		"../assets/vendor/bootstrap/scss/_functions.scss",
    -		"../assets/vendor/bootstrap/scss/_variables.scss",
    -		"../assets/vendor/bootstrap/scss/_mixins.scss",
    -		"../assets/vendor/bootstrap/scss/vendor/_rfs.scss",
    -		"../assets/vendor/bootstrap/scss/mixins/_deprecate.scss",
    ...
    +		"../node_modules/bootstrap/scss/bootstrap.scss",
    +		"../node_modules/bootstrap/scss/_functions.scss",
    +		"../node_modules/bootstrap/scss/_variables.scss",
    +		"../node_modules/bootstrap/scss/_mixins.scss",
    +		"../node_modules/bootstrap/scss/vendor/_rfs.scss",
    +		"../node_modules/bootstrap/scss/mixins/_deprecate.scss",
    ...
  • (Proposed for discussion at 22/02/14 PSC meeting.)

Preview: https://deploy-preview-889--docsydocs.netlify.app/

@chalin chalin added dependencies Pull requests that update a dependency file CI/infra CI & infrastructure labels Feb 14, 2022
@chalin
Copy link
Collaborator Author

chalin commented Feb 14, 2022

/cc @geriom @deining @LisaFC @emckean - as mentioned in the opening comment, let's discuss Hugo modules and this PR during the PSC meeting. Thanks.

@chalin
Copy link
Collaborator Author

chalin commented Feb 14, 2022

I did a quick partial test and I don't believe that this will break #871.

@chalin
Copy link
Collaborator Author

chalin commented Feb 14, 2022

All: The only extra step required on the part of existing users, if/once this is merged, would be this extra command after fetching the docsy submodule:

$ (cd themes/docsy && npm install)  

Or, for projects that are already using NPM, then can add the following script to run the docsy npm install at the same time as the project's:

  "scripts": {
    ...
    "postinstall": "cd themes/docsy && npm install",
    ...
  },

The cleanest approach might be to install docsy as an NPM package, which would be done something like this:

$ npm install github:chalin/docsy#chalin-bs+fa-via-npm-2022-02-14 --save-dev

And then project's wouldn't have to run any extra commands or NPM scripts. But this isn't an approach that I want to fully promote yet since I haven't worked out the details. (And I want to get the Hugo module PR in first.)

Copy link
Collaborator

@geriom geriom left a comment

Choose a reason for hiding this comment

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

Tested it. Works like a charm.

Thank you!

@deining
Copy link
Collaborator

deining commented Feb 15, 2022

Tested it, and could make it work too, after some tweaking.

Experiences/thoughts:

  1. I started off with:
git clone -b chalin-bs+fa-via-npm-2022-02-14 https://github.com/chalin/docsy.git docsy-npm

Since my checkout directory was docsy-npm, I had to adjust userguide/config.toml in that way: theme = "docsy-npm".

  1. Currently, we do have two separate npm-related blocks in README.md:
$ npm install --save-dev autoprefixer
$ npm install --save-dev postcss-cli

and

$ git clone --recurse-submodules --depth 1 https://github.com/google/docsy.git
$ cd docsy/userguide/
$ npm install
$ npm run serve

I needed both to get the site up and running. Can we bring these together?

  1. Question/Proposal: do we want the npm script to pull in the latest version of hugo as well? We are doing this already for the Netlify deploy, AFAICS. There are several choices for pulling in hugo, I tried out hugo-installer yesterday, and it worked great for me.

@geriom
Copy link
Collaborator

geriom commented Feb 15, 2022

  1. Currently, we do have two separate npm-related blocks in README.md:
$ npm install --save-dev autoprefixer
$ npm install --save-dev postcss-cli

and

$ git clone --recurse-submodules --depth 1 https://github.com/google/docsy.git
$ cd docsy/userguide/
$ npm install
$ npm run serve

I needed both to get the site up and running. Can we bring these together?

This is an alternative to those steps. I tried it on a clean linux VM with only node installed, nothing else.

Doing npm install and npm run serve gets the site up and running, no need to neither pull the submodules nor to install go or hugo. This is way more user friendly imo.

After we get this and #871 merged we should revisit our docs and make a decision about how we present the different alternatives.

@chalin chalin force-pushed the chalin-bs+fa-via-npm-2022-02-14 branch from aeb245b to 27cdfbe Compare February 18, 2022 16:34
@chalin
Copy link
Collaborator Author

chalin commented Feb 18, 2022

Thank you both @deining and @geriom for your feedback.

Since my checkout directory was docsy-npm, I had to adjust userguide/config.toml in that way: theme = "docsy-npm".

Right, that's to be expected if checked out to another folder name.

2. Currently, we do have two separate npm-related blocks in README.md

Yes, but those two blocks serve different purposes and cater to two different audiences: the first is for docsy users and the second for docsy contributors.

I agree with @geriom that the README seems ok for now. (Note that I just pushed a commit to remove the --recurse-submodules flag even if it was a harmless noop).

3. Question/Proposal: do we want the npm script to pull in the latest version of hugo as well?

The project is already fetching the version of hugo we need via the hugo-extended package. I'm unsure what more you wanted. (Thanks for the reference to the hugo-installer, I didn't know about it.

@geriom wrote:

After we get this and #871 merged we should revisit our docs and make a decision about how we present the different alternatives.

Agreed.

@chalin chalin force-pushed the chalin-bs+fa-via-npm-2022-02-14 branch from 10195b0 to b47a3a4 Compare March 28, 2022 18:40
@chalin chalin changed the title Bye bye submodules Drop Bootstrap and Font Awesome git submodules Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/infra CI & infrastructure dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants