Corrects SCSS compile extend error

[ckaz]

This corrects the SCSS compile error from line 386 in bootstrap.less introduced in #2779

[ckaz]

Sorry, I had to fix a few more things here.

[demiankatz]

Thanks, @ckaz, looks much better to me now. I'm a bit short on time this week but will try to check this out early next week at the latest.

[demiankatz]

@ckaz, I'll wait and see what happens with @EreMaijala's latest question before I do further testing on this. Sounds like further simplification may be possible, so I'll wait and see so I can test the (hopefully) final version.

[ckaz]

Will remove SCSS-specific comment in favor of @extend

[ckaz]

Sorry, I had to fix a few more things here.

@ckaz, I'll wait and see what happens with @EreMaijala's latest question before I do further testing on this. Sounds like further simplification may be possible, so I'll wait and see so I can test the (hopefully) final version.

Sorry for the delay, we had a chaotic week ...

[ckaz]

I've removed the over-complicated SCSS-rewrite spotted by @EreMaijala -- guess that should be it, then?
(I've merged dev into this PR and got no conflicts.)

[demiankatz]

No worries, @ckaz! I have a long weekend but I'll take another look at this early next week and hopefully get all the SCSS-related stuff wrapped up then.

[demiankatz]

@ckaz, sorry for not realizing this sooner, but this PR seems to rely on making changes to vendor files, which is something we shouldn't do, because they're third-party files that are not under the control of this project. If we make changes there, then whenever we upgrade the third-party library, we'll lose our changes. Perhaps that's not such an issue here because bootstrap is at end of life and should be replaced soon anyway, but I still in principle don't particularly like that solution.

Additionally, code quality checking and auto-conversion are not applied to vendor files, so it appears that this has caused the LESS and SCSS versions of these files to get out of sync with each other.

Is there a simpler way to solve this problem?

[demiankatz]

@ckaz, after talking to @EreMaijala, I suspect the simplest solution here may simply be to duplicate the vendor styles in a place that we control. I think in this instance the redundancy would be justified.

[ckaz]

@ckaz, after talking to @EreMaijala, I suspect the simplest solution here may simply be to duplicate the vendor styles in a place that we control. I think in this instance the redundancy would be justified.

You are right, of course. Just committed my changes. I still can't test this locally in the browser, but the files seem to compile fine.
Pls. let me know if that works for you.

[ckaz]

I've created a custom 'mixins' folder and moved the changes there. SCSS seems to compile fine.

[ckaz]

Still confused by the review mechanisms here. I need to check all files and then press "Review changes", right?

[demiankatz]

Thanks, @ckaz, I'll give this another look very soon. Regarding the review mechanism, you should just be able to click the icon next to my name in the "Reviewers" section at the top-right of the pull request to re-request a review from me. It shouldn't be necessary to select any files.

[demiankatz]

@ckaz, there were a bunch of remaining issues here: some vendor files were still being modified, the lessToSass process wasn't correctly converting the new mixins directory, and there were some apparently hand-converted SCSS mixins that seemed out of sync with the Less versions. I've tried to clean this all up and have pushed up a new version. Can you take a look at this and let me know if it seems okay from your end?

[demiankatz]

@ckaz, I just tested this, comparing the impacted dropdown menus between the dev branch and this branch, using both LESS and SCSS compilation. The dropdown menus themselves are consistent in all situations, so I think this PR is a success. (I should note that I only checked the bootprint3 theme due to time limitations, but I have no reason to believe the others would differ).

I have one small suggestion regarding a comment (see below). I'm happy to merge this once you've had a chance to review and approve my own changes, and adjust the comment if you so desire.

I should also note that some other aspects of styles do appear to be inconsistent between LESS and SCSS: the "format" pills were different colors, and the "pressed" state of the button that triggered the drop-down menu looked different. I suspect that these problems are unrelated to the changes here, however.

[ckaz]

Looks good to me. Did change the comment.

[demiankatz]

Thanks for the updates -- looks good now, so I will merge shortly.

I agree that some follow-up on the SCSS inconsistencies might be a good idea, and your theory about the cause seems likely to be correct. If you don't have time to look at this any time soon, let me know and I'll open a JIRA ticket so we don't lose track.

[ckaz]

Thanks for the updates -- looks good now, so I will merge shortly.

I agree that some follow-up on the SCSS inconsistencies might be a good idea, and your theory about the cause seems likely to be correct. If you don't have time to look at this any time soon, let me know and I'll open a JIRA ticket so we don't lose track.

I'll be on leave for most of July, so a JIRA ticket would be the best option, I guess. Cheers!

[demiankatz]

Thanks, @ckaz! I have opened VUFIND-1619 so we don't lose track of this. I've pinned it to release 10.0, because I don't think it's an urgent priority, and it's possible needs will change depending on overall theme redesign strategy... but it's important to keep it in the discussion for now.

I hope you enjoy your leave!