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

#8986 Image before Title still respects Title metadata #9162

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

StephanMeijer
Copy link
Contributor

@StephanMeijer StephanMeijer commented Oct 31, 2023

The way I implemented this:

  1. First comes regular stuff, like images or anything else
  2. Then a block of meta comes including empty paragraphs
  3. Then regular stuff like images and paragraphs again

Then it's processed in this order:

  1. Meta without any empty paragraphs
  2. Non-meta
    a. First block of regular stuff
    b. All empty paragraphs from within Meta
    c. Second block of regular stuff

Visualisation of processing logic


Our internal ID: NLDOC-837

,Para [Strong [Str "No",Space,Str "table",Space,Str "of",Space,Str "figures",Space,Str "entries",Space,Str "found."]]
,Header 1 ("introduction",[],[]) [Str "Introduction"]
,Para [Str "Nothing",Space,Str "to",Space,Str "introduce,",Space,Str "yet."]]
Pandoc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-10-31 at 14 27 39

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what to do with this. It is entirely logical this is being converted to a Title (meta) so I would prefer letting that meaning just sit in there and maybe change the example.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you explain more fully? Are you saying that the Title style is used for both the document title and the title of the table of contents section?

@StephanMeijer StephanMeijer force-pushed the feature/8986-image-meta branch from ff98b1b to a87f67e Compare October 31, 2023 13:50
@StephanMeijer StephanMeijer force-pushed the feature/8986-image-meta branch from a87f67e to 05584c0 Compare October 31, 2023 13:50
@StephanMeijer StephanMeijer changed the title Draft: #8986 Test for image before title #8986 Test for image before title Oct 31, 2023
@StephanMeijer
Copy link
Contributor Author

@jgm Feel free to review. Also asked my collegeau Paul to review.

@StephanMeijer StephanMeijer changed the title #8986 Test for image before title #8986 Image before Title still respects Title metadata Oct 31, 2023
Copy link

@pkrijgsman pkrijgsman left a comment

Choose a reason for hiding this comment

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

Looks great, this should be a fix for the issues that we have.

@StephanMeijer
Copy link
Contributor Author

@jgm responded at #9170 (comment)

This approach (skipping images) feels a bit ad hoc to me.

I'm thinking it might be preferable to allow items with class Title, Author, or Date to be counted as metadata no matter whether they occur in the document. In the case of Title or Date, only the first would be counted as part of metadata if there are multiple. In the case of multiple Author, they could all be included in a list.

@StephanMeijer
Copy link
Contributor Author

@jgm I changed the PR based on your feedback in my other PR.

@StephanMeijer StephanMeijer force-pushed the feature/8986-image-meta branch from 3a96df4 to d20c61b Compare November 6, 2023 18:31
@jgm
Copy link
Owner

jgm commented Nov 7, 2023

Actually, my suggestion was to replace all the "splitting" code with the simple behavior I mentioned. Was trying to make it simpler, not more complicated.

@StephanMeijer
Copy link
Contributor Author

@jgm I think I misunderstood what you mean then, as I implemented (I think) exactly what you described. Could you elaborate?

Comment on lines +199 to +201
maybeToList :: Maybe a -> [a]
maybeToList Nothing = []
maybeToList (Just x) = [x]
Copy link
Owner

Choose a reason for hiding this comment

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

I think there's a function like this in Data.Maybe

@jgm
Copy link
Owner

jgm commented Nov 7, 2023

I think you do implement what I suggested. But I guess I'd had in mind what might be a simpler approach. Instead of going through all the bp's and splitting them into metadata and non-metadata, I thought it would be simpler to simply keep a state variable for metadata and populate it as we go through the bp's, sequentially. Wouldn't that allow us to avoid all the splitting code at the beginning, while producing the same result? Let me know if that makes sense.

@StephanMeijer
Copy link
Contributor Author

@jgm it absolutely makes sense!

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.

3 participants