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

feat: make Buckram compatible with latest SCSSPHP #304

Merged
merged 6 commits into from
Jan 3, 2023
Merged

Conversation

greatislander
Copy link
Collaborator

@greatislander greatislander commented Dec 19, 2022

Context for the issue

When a book author selects the position for running content in PDF outputs, a complex series of rules must be applied. For example, if one indicates that:

  • the page number on the first page of a chapter should be on the bottom outside corner
  • the page number on the remaining pages of the chapter should be on the bottom center
  • chapters can start on either the left or the right page

…the CSS needs to respond appropriately:

  • styling the @bottom-left area when a chapter begins on the left page
  • styling the @bottom-right area when a chapter begins on the right page
  • styling the @bottom-center area on all other left/right pages in the chapter.

In all earlier implementations of Buckram, such rules were composed using the following syntax within a few different mixins:

#{$content-position} {
	// Rules go here.
}

$content-position would be something like @bottom-center and the #{} syntax is Sass's method of string interpolation.

HOWEVER. That's not valid Sass. You can't (as far as I can tell) generate an entire selector from an interpolation rule. Trying to do this with @bottom-center throws a critical error and the Sass fails to compile. A bug in SCSSPHP <= 1.1.1 was allowing this non-standard SCSS to compile, but the bug was resolved in SCSSPHP 1.2 which broke Buckram's SCSS.

What this PR does

Instead of using interpolation, I've created a helper mixin called position which, when passed a position value such as @bottom-left, @top-left, etc, will wrap content in it. So for example:

@include position('@top-left') {
	text-align: left;
}

Compiles to:

@top-left {
	text-align: left;
}

This resolves the incompatibility with SCSSPHP (and the SCSS standard) and allows SCSSPHP to be safely updated to the current version.

Testing

To verify that the page structure still works as expected, I completed the following tests:

  1. Using PHP 7.4 and SCSSPHP 1.1.1 and the dev version of Buckram, I ran:
vendor/bin/pscss tests/source/styles/prince-toc-left.scss > before.css
  1. Using PHP 8.0 and SCSSPHP 1.11.0 and this PR version of Buckram, I ran
vendor/bin/pscss tests/source/styles/prince-toc-left.scss > after.css

You can compare the output files in the attached Zip archive.

Related issues

Resolves pressbooks/pressbooks#2281.

@private-packagist
Copy link

composer.lock

Dev Package changes

Package Operation From To Changes
scssphp/scssphp upgrade 1.1.1 v1.11.0 diff

Settings · Docs · Powered by Private Packagist

@greatislander greatislander marked this pull request as ready for review December 19, 2022 17:35
@greatislander greatislander self-assigned this Dec 19, 2022
@greatislander greatislander changed the title feat: make Buckram compatible latest SCSSPHP feat: make Buckram compatible with latest SCSSPHP Dec 19, 2022
@SteelWagstaff SteelWagstaff marked this pull request as draft December 19, 2022 18:36
@greatislander greatislander marked this pull request as ready for review December 20, 2022 02:55
@SteelWagstaff
Copy link
Member

@arzola and @fdalcin here's a short video explaining context and next steps for reviewing/testing this change: https://us02web.zoom.us/rec/play/eA1S9yeGaO2EdBFsatMdU4T_OJAydWYSmr_JFkBdJlaoN5eYLoLDZLfacac27vH-OUMIv6BsE1vUWmQ.jJnI6stDjTPs4kOx?continueMode=true

@arzola
Copy link
Contributor

arzola commented Jan 3, 2023

I'm testing this and I noticed the following behaviour I think is related to the new UTF-8 ascii/unicode handling in the library.

This is with PHP 7.4

#toc .front-matter, #toc .part {
	margin-left: 1cm
}

#toc .part {
	margin-right: 1.5cm
}

#toc .part a:before {
	content: "Part" "\A0" counter(part, upper-roman) ".\A0";
	display: inline-block;
	text-align: left
}

#toc .chapter a:before {
	content: "" counter(chapter) ".\A0\A0\A0"; //ASCII representation
	display: inline-block;
	float: left;
	overflow-wrap: normal;
	text-align: right;
	width: 1cm
}

PHP 8.0

@charset "UTF-8";
#toc .front-matter, #toc .part {
	margin-left: 1cm
}

#toc .part {
	margin-right: 1.5cm
}

#toc .part a:before {
	content: "Part" " " counter(part, upper-roman) ". "; // No more ASCII representation
	display: inline-block;
	text-align: left
}

#toc .chapter a:before {
	content: "" counter(chapter) ".   ";
	display: inline-block;
	float: left;
	overflow-wrap: normal;
	text-align: right;
	width: 1cm
}

I think this is the expected behaviour right? @greatislander a non breaking space

image

@SteelWagstaff
Copy link
Member

@arzola yes -- I think the desired outcome is the insertion of non-breaking spaces into the content property.

@arzola arzola merged commit eb087ca into dev Jan 3, 2023
@SteelWagstaff SteelWagstaff deleted the fix/scss-bug branch January 3, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make compatible with latest working version of SCSSPHP
3 participants