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

Separate custom node handlers for block and inline #7083

Merged
merged 1 commit into from Oct 2, 2023

Conversation

knuesel
Copy link
Contributor

@knuesel knuesel commented Sep 29, 2023

Description

This addresses #6940 by storing custom block and inline handlers in seperate tables. This way it is possible for the same class name (content-visible) to have one handler for divs and another for spans.

Currently this PR doesn't add a handler for Spans, so they don't get a custom node wrapper, but they still get a working content-hidden treatment, like code blocks. Without this PR, conditional spans cause an error.

If desired, I can add custom nodes for inlines (to wrap conditional spans), and wrap conditional code blocks using the existing ConditionalBlock.

I'll be happy to add some tests and documentation if the general idea is approved, and after it's decided what to do about custom nodes for conditional spans and code blocks.

Checklist

I have (if applicable):

  • filed a contributor agreement.
  • referenced the GitHub issue this PR closes
  • updated the appropriate changelog

@cscheid
Copy link
Collaborator

cscheid commented Sep 29, 2023

(Let me just start by saying I'm very impressed that you were willing and able to dig in this part of the code!)

Copy link
Collaborator

@cscheid cscheid left a comment

Choose a reason for hiding this comment

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

A few stylistic requests:

Lua code is supposed to be snake_case. We're slowly improving our codebase (which as you will certainly have experienced is wildly inconsistent). However, we're trying to not make matters worse with new commits. namedHandlers -> handlers is an improvement, but can you make byAstName into by_ast_name?

This allows storing different handlers for blocks and inlines for the
same class name.
@knuesel
Copy link
Contributor Author

knuesel commented Oct 2, 2023

Thanks! I've renamed the field to by_ast_name.

@cscheid
Copy link
Collaborator

cscheid commented Oct 2, 2023

This looks very good. There's one small renaming I would want to do: table is a Lua built-in, and using it as a local name is asking for future trouble. In addition, what you're passing there is a key, and not a table, so I'd use key instead. But I don't want to ask you for yet another change, so I'm going to merge as is and make the tiny fix myself.

Thanks again. Looking to the future, since I seem to recall you want conditional spans.

Tests: we don't have great infrastructure for testing our Lua codebase in isolation (because it all runs inside Pandoc). We do much better with end-to-end render tests, and those tend to cover basic features like this one. It's not ideal, but it's better than nothing.

With that said, for document-level features, we will 100% want tests that cover expected behavior. You'll find a number of those test documents in our tests/docs subdirectory. Specifically, you'll probably want to use the in-document testing we have in YAML under _quarto: tests:. Hopefully that's enough for you to go on, but please reach out if you need more assistance.

@cscheid cscheid merged commit 4ec4ae3 into quarto-dev:main Oct 2, 2023
47 checks passed
cscheid added a commit that referenced this pull request Oct 2, 2023
@cderv
Copy link
Collaborator

cderv commented Oct 2, 2023

Thanks @knuesel !

Should #6940 be closed by this PR ?

Just in case it should have been (github closing keyword did not worked here)

@knuesel
Copy link
Contributor Author

knuesel commented Oct 2, 2023

Good point for the table name (don't hesitate to ask for as many changes as you want next time, I don't mind!).

Regarding conditional spans: I think this PR was all I needed for my projects: it's enough to get [...]{.content-visible ...} working.

Thanks for the tips on tests. For the future of conditional nodes I was wondering if you eventually want custom nodes for all of them. Currently content-visible is working with divs, code blocks and now also spans, but only the divs get wrapped in a ConditionalBlock. We could wrap code blocks in those too, and define a new custom node type (with name ConditionalInline I guess) to wrap spans. But maybe it's better to wait until someone actually needs it.

@cderv yes thanks for the reminder, it's closed now.

@cscheid
Copy link
Collaborator

cscheid commented Oct 2, 2023

Regarding conditional spans: I think this PR was all I needed for my projects: it's enough to get [...]{.content-visible ...} working.

Ok. This is great, I thought we would have needed additional code! In that case we should 100% add document-level tests with expected outputs. Would you be willing to add some?

But maybe it's better to wait until someone actually needs it.

Agreed. It's not even clear to me that we need ConditionalBlock. I wrote that in part to test the (at the time new) AST infrastructure.

@knuesel
Copy link
Contributor Author

knuesel commented Oct 3, 2023

Sure, I'll prepare some tests.

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.

None yet

3 participants