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

Permit the \page Token to accept {} values. #3899

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dbolack-ab
Copy link
Collaborator

@dbolack-ab dbolack-ab commented Nov 19, 2024

Description

This PR allows users to apply style, class, and attribute values via the inline {} format to the base page for common inheritance.

To Do:

  • Fix Attributes, They are not being inserted
  • Add Tests

Solves #3901

Usage:
Similar to an inline single mustache.

\page{wide,banna=fail,color:black}

*Reviewers, refer to this list when testing features, or suggest new items *

  • Verify new features are functional
    • Adding a style includes a style on the
      instance.
    • Adding a class includes a class on the
      instance.
    • Adding an HTML Attribute adds the attribute on the
      instance.
    • Mixed forms of above work as expected
  • Verify old features have not broken
    • \page works without values.
  • Test for edge cases / try to break things
    • Ensure CSS Properties ( --FOO ) can be set.
  • Identify opportunities for simplification and refactoring
  • Check for code legibility and appropriate comments
Copy this list

@calculuschild
Copy link
Member

Is there a concern that this doesn't apply to the very first page which doesn't have a /page break?

@dbolack-ab
Copy link
Collaborator Author

Is there a concern that this doesn't apply to the very first page which doesn't have a /page break?

No, though the included code also tolerates putting a \page as the very first line for the occasion that is desirable ( this functionally moves \page to being a START-OF-PAGE instead of END-OF-PAGE marker.

@calculuschild
Copy link
Member

Would you mind creating an issue for this PR? So we have a place to document and discuss the problem this is aiming to solve.

@5e-Cleric
Copy link
Member

Is there a concern that this doesn't apply to the very first page which doesn't have a /page break?

No, though the included code also tolerates putting a \page as the very first line for the occasion that is desirable ( this functionally moves \page to being a START-OF-PAGE instead of END-OF-PAGE marker.

I cannot see this working in the deployment, although the rest seems to work fine.

While this syntax works for us, it is also worth mentioning that codemirror stops processing that line as a .pageLine, so we have no way of targeting that element for code highlight.

IF we did it like with tables, that you need the injection to be in the next line:

\page
{background:red}

That should preserve the code hightlight, and be somewhat intuitive and consistent.

I wish we could keep it in the line though

@dbolack-ab
Copy link
Collaborator Author

Is there a concern that this doesn't apply to the very first page which doesn't have a /page break?

No, though the included code also tolerates putting a \page as the very first line for the occasion that is desirable ( this functionally moves \page to being a START-OF-PAGE instead of END-OF-PAGE marker.

I cannot see this working in the deployment, although the rest seems to work fine.

What can you not see?

Regarding the syntax highlighting - yes, that's not there yet.

@5e-Cleric
Copy link
Member

5e-Cleric commented Nov 23, 2024

What can you not see?

adding {background:red} at the beggining of the document does not work.

@dbolack-ab
Copy link
Collaborator Author

What can you not see?

adding {background:red} at the beggining of the document does not work.

No. It won't. This syntax only works on a \page line as described in usage.

@Gazook89
Copy link
Collaborator

For the first page, @5e-Cleric :

image

@Gazook89
Copy link
Collaborator

Also, I'll just add this discussion that was on this same topic, before we started using named spans and :has() css selector: #1489. I don't know if it adds anything but it bothered me that I could half-remember talking about this but not the details.

@calculuschild
Copy link
Member

the included code also tolerates putting a \page as the very first line for the occasion that is desirable ( this functionally moves \page to being a START-OF-PAGE instead of END-OF-PAGE marker.

Is there any risk of this breaking existing brews? I'm not sure if there would be any documents out there depending on a \page at the start to make a blank page. Probably not common, right?

@5e-Cleric
Copy link
Member

Is there any risk of this breaking existing brews? I'm not sure if there would be any documents out there depending on a \page at the start to make a blank page. Probably not common, right?

This should break no document. I have never seen or heard, or can think of any good reason to have an empty page at the top.

@dbolack-ab
Copy link
Collaborator Author

the included code also tolerates putting a \page as the very first line for the occasion that is desirable ( this functionally moves \page to being a START-OF-PAGE instead of END-OF-PAGE marker.

Is there any risk of this breaking existing brews? I'm not sure if there would be any documents out there depending on a \page at the start to make a blank page. Probably not common, right?

This code (based on my master page code ) uses a slightly smarter function in place of the src/split('\page\n') we use now. Part of its logic is if element[0] in the split array is empty, it is dropped from the array. This allows backward compatibility with the current pattern and allows the use of \page on the first line for enhanced page settings like this or master pages or whatever else we might think of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

5 participants