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

Refactor Markdown classes #651

Merged
merged 77 commits into from
Nov 8, 2022
Merged

Refactor Markdown classes #651

merged 77 commits into from
Nov 8, 2022

Conversation

caendesilva
Copy link
Member

@caendesilva caendesilva commented Nov 7, 2022

Refactors Markdown related classes as a lot of these are pretty much as old as the framework itself.

Notable changes:

@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #651 (13ea227) into master (2bb888d) will not change coverage.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##              master      #651   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity      1020      1024    +4     
===========================================
  Files            126       126           
  Lines           2544      2551    +7     
===========================================
+ Hits            2544      2551    +7     
Impacted Files Coverage Δ
...res/Documentation/SemanticDocumentationArticle.php 100.00% <100.00%> (ø)
...ackages/framework/src/Markdown/Models/Markdown.php 100.00% <100.00%> (ø)
.../framework/src/Pages/Concerns/BaseMarkdownPage.php 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@what-the-diff
Copy link

what-the-diff bot commented Nov 7, 2022

  • Changed the visibility of Markdown::$body from public to protected.
Click to show WTD guesses that were not quite right
  • Added a new method, Markdown::compile(), which compiles markdown into HTML and returns it as a string.

  • Renamed the existing compile() method on DocumentationPage to render(). This is because we now have two methods named "compile" in this class: one that renders documentation pages (which was previously called "render") and another that compiles markdowns into HTML strings (the newly added function). The old compile() has been renamed so there's no confusion between these two functions with similar names but different purposes/signatures/return types etc...

  • Updated all references to $markdown->body throughout Hyde\Framework\Testing\Feature namespace accordingly; iow, changed them from $page->markdown->body -> (string) $page->markdown. I did not update any other usages of body outside this test suite since they are either already using getters or don't need updating for some reason - eg., if you look at how \Hyde\Pages uses body(), it doesn't actually use its return value anywhere except when calling strlen(); instead, what matters here is just whether or not an empty string gets returned by body(). So even though technically speaking we should be changing those calls too, doing so would add unnecessary complexity without adding much benefit IMO :)

@caendesilva caendesilva marked this pull request as ready for review November 8, 2022 17:56
@caendesilva
Copy link
Member Author

This is why I am very happy we have Percy installed. It seems like this has broken parts of the custom Markdown system. Some Markdown parts are not being compiled, it seems to be the custom colored blockquotes and tables, so I think somewhere we lost passing data to the Markdown compiler about the rendered page so we're using the simple Markdown converter instead of the dynamic one.

@caendesilva caendesilva merged commit 6c7933c into master Nov 8, 2022
@caendesilva caendesilva deleted the refactor-markdown-classes branch November 8, 2022 18:31
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