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

Use guides to parse and render markdown #632

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

Conversation

jaapio
Copy link
Contributor

@jaapio jaapio commented Oct 22, 2024

This is an effort to remove the static website generator project as a dependency. In it's current state guides-markdown does not support all we need to replace the markdown rendering, so I'm working on that while fixing this project as well.

With linkable headings:

image

And nice code blocks like the docs have:

image

@jaapio jaapio force-pushed the guides-markdown branch 2 times, most recently from 8acd96e to 99767ad Compare October 26, 2024 08:44
@jaapio jaapio marked this pull request as ready for review October 26, 2024 08:50
@SenseException
Copy link
Member

Nice. I'll test this when I get the time. Can you please add a test for the newly introduced class?

Copy link
Member

@SenseException SenseException 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. Code snippets look better now and headlines have anchors. I found a few anomalies in the rendering of markdown pages though:

Ordered and unordered lists gaps

Because of a <p> tag that is rendered with the <li> and <ol> ones do lists have a gap of a paragraph for all markdown pages.

(https://www.doctrine-project.org/2016/01/05/dbal-2-5-4-and-2-4-5.html)
Auswahl_006

Missing links and anchor

In at least one case aren't the links in a headline rendered and the anchor can't be used. The anchor also looks wrong being dark and bigger than on other pages. I assume this might have something to do with the brackets in the headline.

(https://www.doctrine-project.org/2021/11/26/dbal-3.2.0.html)
Auswahl_005

@jaapio
Copy link
Contributor Author

jaapio commented Nov 7, 2024

I checked the headlines in the blogpost that look malformed, and those are actaully links within the headline. And that makes it look very strange.

## Support for `psr/log` ([#4967](https://github.com/doctrine/dbal/pull/4967))

As the headline is rendered as a link, and includes a link.... yeah that's where things get really ugly...
Honestly I don't know how to solve that, do you want me to update those posts? And transform the link to the issue to a link directly below the headline?

@jaapio
Copy link
Contributor Author

jaapio commented Nov 7, 2024

The issue with the paragraphs is covered in phpDocumentor/guides#1168

@jaapio
Copy link
Contributor Author

jaapio commented Nov 7, 2024

Fixed the list:

image

@jaapio
Copy link
Contributor Author

jaapio commented Nov 7, 2024

Although I do agree with you tests are very much needed the added class in this PR is very hard to test. Basically we just glue some external parts together. Adding a test would be nothing more than a very complicated mock and it will basically test nothing as the parser is an external project and the noderenderer is as well.

The rest will be covered by static analysis.

@SenseException
Copy link
Member

do you want me to update those posts? And transform the link to the issue to a link directly below the headline?

Yes, that would probably be the best.

Adding a test would be nothing more than a very complicated mock

There are also functional tests, like in tests/FunctionalTest.php where a service can be used. The content for the rendering itself can be controlled with the injected Doctrine\StaticWebsiteGenerator\SourceFile\SourceFile object.

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.

2 participants