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

expand childpages if converted to pandoc #100

Merged
merged 3 commits into from
Jan 30, 2023
Merged

expand childpages if converted to pandoc #100

merged 3 commits into from
Jan 30, 2023

Conversation

sHermanGriffiths
Copy link
Contributor

@sHermanGriffiths sHermanGriffiths commented Jan 27, 2023

Author's Checklist

Before assigning reviewers, step through the following checklist. Check each item once it's completed. If an item is skipped, write an explanation below the item.

  • Construction: Review the code diff and confirm there's no dead code, debug code, or unnecessary comments.
  • Requirements: Review the requirements in the issue for this PR and confirm they're all implemented.
  • Architecture: Confirm that the changes to the code are consistent with the existing architecture.
  • SOUP/OTS Software: Any new dependencies have appropriate licenses and are documented or pinned in requirements files.
  • Unit Test: Atomic unit tests have been added, if appropriate.
    N/A
  • End-to-End Tests: End-to-end tests have been extended or updated, if appropriate.
    N/A
  • Low-level Documentation: Comments or doc strings for affected code have been updated.
    N/A
  • High-Level Documentation: The README is updated, as appropriate.
    N/A
  • Change Log: New features or bug fixes have been added to the change log in the README.
    N/A

n2y/blocks.py Outdated
elif block_type == ChildPageBlock:
for b in blocks:
result = b.to_pandoc()
title = result[0][0]['title'][0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice idea! I think it makes a lot of sense to expand out sub pages. I would actually not show the page title as an H1, and would just show the page's content in place. This would work identically to linked pages then, which we use (or have a plugin) that expands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, I only did it as a debug because this subpage was converting to pandoc as a Pandoc block inside of another Pandoc block and that's against protocol. Is there another way you'd solve that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was due to a weird thing Emily did in the templates. I just fixed it in the templates! That said, I think expanding the page (without the title) is a good idea.

n2y/blocks.py Outdated
@@ -67,6 +67,10 @@ def children_to_pandoc(self):
for block_type, blocks in groupby(self.children, lambda c: type(c)):
if issubclass(block_type, ListItemBlock):
pandoc_ast.append(block_type.list_to_pandoc(blocks))
elif block_type == ChildPageBlock:
Copy link
Member

Choose a reason for hiding this comment

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

Could we just add a condition to the if statement on Line 77 below rather than having a separate elif for this?

if isinstance(result, list) or block_type == ChildPageBlock:

I think we'd still get the desired functionality right (double check this please)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not the same because ChildPageBlock.to_pandoc() results in a Pandoc block which will cause an error because all of the pandoc will eventually end up in another Pandoc block. that's why result[1] is selected on line 73. I could bring this logic down into that iteration though

Copy link
Member

@bimbashrestha bimbashrestha left a comment

Choose a reason for hiding this comment

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

Nice work @sHermanGriffiths!

@sHermanGriffiths sHermanGriffiths merged commit a7b29fc into main Jan 30, 2023
@sHermanGriffiths sHermanGriffiths deleted the childpage branch January 30, 2023 20:50
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