Skip to content

Replace grunt with NPM scripts #2624

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

Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

crhallberg
Copy link
Contributor

@crhallberg crhallberg commented Nov 11, 2022

Sass

Less

  • This should be implemented into a feature branch with lessToSass.

Tests

  • Remove lessToSass.

TODO

  • Update relevant wiki documentation before merging
  • Resolve VUFIND-1514 when merging

@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2022

This pull request introduces 7 alerts when merging 05a02e4 into 70e33c7 - view on LGTM.com

new alerts:

  • 7 for Duplicate character in character class

@demiankatz
Copy link
Member

Thanks for getting this started, @crhallberg! I've just added a TODO to remind us to update documentation when this is done.

@crhallberg
Copy link
Contributor Author

crhallberg commented Nov 18, 2022

After sitting down with Demian for a while, we determined that there's no good way to include the ability to exclude files.

Using FileImporters would involve adding a prefix or other nonce to every import in order to bypass the default loader. This could work, moving the parent path logic to the function and allow people to exclude files. This is not a solution either of us was happy with and it was unclear if this would work in the bootstrap theme. If this solution appeals to you, check this diff for all of the code I removed, which would be a good starting point.

The other method discussed was to use a tool like purgeCSS in order to remove a lot of the bloat Bootstrap adds. This would work but it seems that the best option is to just quickly offer a framework agnostic theme. So these patches were set aside in favor of finishing the work on that.

@aleksip
Copy link
Contributor

aleksip commented Nov 21, 2022

If we are going for framework agnostic, maybe dropping Sass and Less and using "future CSS" with postcss-preset-env can be reconsidered?

@ckaz
Copy link
Contributor

ckaz commented Nov 23, 2022

Hi Chris,

After sitting down with Demian for a while, we determined that there's no good way to include the ability to exclude files.

That's a shame. However, would it be possible to rework the SCSS file that generates the imports then? I mean, in the bootstrap3-theme, the bootstrap.scss and so on:

`// Core variables and mixins
@import "bootstrap/variables";
@import "bootstrap/mixins";

// Reset and dependencies
@import "bootstrap/normalize";
@import "bootstrap/print";
@import "bootstrap/glyphicons";

// Core CSS
@import "bootstrap/scaffolding";
@import "bootstrap/type";
@import "bootstrap/code";
@import "bootstrap/grid";
@import "bootstrap/tables";
@import "bootstrap/forms";
@import "bootstrap/buttons";

etc etc
`

My colleague Alex came up with a concept which allows us to import customizations into library sites using variables:

First, he created mixins out of the customization scss files, then he added @import rules and introduced a default variable which can be overridden in every individual library like so:

@import 'activate-on-demand-custom-element-name';
@if $activate-on-demand-custom-element-name {
@include plugin-to-be-activated;
}

and in the variables file:

$activate-on-demand-custom-element-name: false !default;

Or something along that vein. We'd be particularly keen on getting rid of gylphicons, being able to choose whether to import our own FontAwesome or similar font icon library, exclude/include jumbotron, thumbnails and so forth.

I'm aware that this is most likely not properly thought out yet.

Cheers! Claas

@crhallberg
Copy link
Contributor Author

@aleksip If we are going for framework agnostic, maybe dropping Sass and Less and using "future CSS" with postcss-preset-env can be reconsidered?

I think PostCSS would be a great addition! I was hoping to use PostCSS to parse Less and Sass but the plugins don't work that way. By switching to npm scripts, I hope using PostCSS or any other system would be much easier! Even better: we would have a feature branch on GitHub for each of the major pre-processors.

@crhallberg
Copy link
Contributor Author

crhallberg commented Dec 2, 2022

@ckaz I like the idea of a config.scss to enable/disable includes.

@demiankatz demiankatz added this to the 9.0 milestone Dec 5, 2022
@demiankatz demiankatz modified the milestones: 9.0, 10.0 Mar 22, 2023
@demiankatz demiankatz mentioned this pull request May 1, 2023
1 task
EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Aug 11, 2023
@demiankatz demiankatz added the tooling pull requests that change project tooling label Nov 7, 2023
@demiankatz
Copy link
Member

I'm marking this one as "on hold" until #3222 is finished, because replacing bootstrap may change the tooling requirements; it probably makes sense to figure out our destination before we try to get there. (But feel free to disagree with me if anyone wants to move this forward sooner).

@demiankatz demiankatz modified the milestones: 10.0, 11.0 May 7, 2024
@demiankatz
Copy link
Member

Moving milestone as per Community Call discussion today.

@crhallberg
Copy link
Contributor Author

In the future, I think we can use nodemon with external configs to make the watch commands easier to control and config.

@demiankatz demiankatz changed the base branch from dev to dev-11.0 August 27, 2024 10:45
@demiankatz demiankatz changed the base branch from dev-11.0 to dev November 1, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement on hold tooling pull requests that change project tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants