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

[VUFIND-1741] Remove bootstrap3, bootprint3 and sandal themes #4192

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

EreMaijala
Copy link
Contributor

@EreMaijala EreMaijala commented Jan 20, 2025

TODO

@EreMaijala EreMaijala requested a review from demiankatz January 20, 2025 11:14
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this, @EreMaijala -- looking good so far, but see below for a few minor suggestions and comments.

"watch:less": "grunt watch:less",
"build-dev:less": "grunt lessdev && npm run lessToSass",
"watch-dev:less": "grunt watch:lessdev",
"build:css": "npm run build:scss",
Copy link
Member

Choose a reason for hiding this comment

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

Since we're ripping a bunch of stuff out here, we should update the npm wiki page accordingly. I've added a TODO checkbox for the task. I'm not sure if it's best to keep existing documentation but flag things as deprecated/removed, or to fully simplify it. I guess we should probably maintain some legacy documentation for a while, though my gut just wants to get rid of it quickly. :-)

*
* @return string
*
* @SuppressWarnings(PHPMD.UnusedLocalVariable)
*/
protected function makeRelative($css, $less)
protected function makeRelative($css, $scss)
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, this method is never used. Should we just remove it, or am I missing something?

; sandal = bootstrap3 theme with a "flat" styling applied (a newer look
; than bootprint3).
theme = bootprint3
; bootstrap5 = HTML5 theme using Bootstrap 5 with minimal styling.
Copy link
Member

Choose a reason for hiding this comment

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

Rather than updating all of these test fixtures, I wonder if we should try to reduce them to contain less information, so they only cover the things we're actually testing. What do you think? I'm open to trying to help with that if you'd like me to!

@@ -65,7 +65,6 @@
<property name="solr_pid_file" value="${localdir}/solr-${solr_port}.pid" />
<property name="marc_bib_dir" value="${srcdir}/tests/data" /><!-- directory containing MARC bib records for test environment -->
<property name="marc_authority_dir" value="${marc_bib_dir}/authority" /><!-- directory containing MARC authority records for test environment -->
<property name="bootstrap3_theme" value="bootstrap3" /><!-- base Bootstrap 3 theme (used for theme update tasks) -->
<property name="bootstrap5_theme" value="bootstrap5" /><!-- base Bootstrap 5 theme (used for theme update tasks) -->
Copy link
Member

Choose a reason for hiding this comment

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

We can also delete the bootstrap5_theme property; it's only used by tasks that have now been removed.

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.

2 participants