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

Fix problems with table rendering #1145

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

Fix problems with table rendering #1145

wants to merge 4 commits into from

Conversation

benjagm
Copy link
Collaborator

@benjagm benjagm commented Dec 1, 2024

Summary

It has been notified in slack an issue with the markdown tables rendering. This PR fixes it.

@benjagm benjagm requested a review from a team as a code owner December 1, 2024 17:36
Copy link

github-actions bot commented Dec 1, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
website ✅ Ready (View Log) Visit Preview 4c0db8f

Copy link

codecov bot commented Dec 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (8676fc2) to head (4c0db8f).
Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1145   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          373       373           
  Branches        94        94           
=========================================
  Hits           373       373           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@karenetheridge
Copy link
Member

The preview (https://json-schema-org-benjagm-fix-3r57.website-2v2.pages.dev/) seems to show a different page.

@benjagm
Copy link
Collaborator Author

benjagm commented Dec 1, 2024

https://json-schema-org-benjagm-fix-3r57.website-2v2.pages.dev/

Hi Karen. The url is a whole new deployment of the website. To reach to the specific page you need to make all the navigation steps. In this case this is the page you reported: https://json-schema-org-benjagm-fix-3r57.website-2v2.pages.dev/draft-06/json-schema-release-notes

Copy link
Member

@DarhkVoyd DarhkVoyd left a comment

Choose a reason for hiding this comment

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

Thanks for this. Could we also address the link rendering bug and some inconsistencies in back-ticks markdown highlighting?

Screenshot 2024-12-02 at 6 54 36 PM

Screenshot 2024-12-02 at 6 55 06 PM

Copy link
Member

@karenetheridge karenetheridge 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 comments follow...

@karenetheridge karenetheridge self-requested a review December 5, 2024 17:16
Copy link
Member

@karenetheridge karenetheridge left a comment

Choose a reason for hiding this comment

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

I clicked around a bit more...

This file looks a bit off: https://json-schema-org-benjagm-fix-3r57.website-2v2.pages.dev/draft/2020-12/release-notes (at "$dynamicRef and $dynamicAnchor").

Also selecting the headings results in the underlying html ending up in the copy buffer, which is really weird! but I don't know if that's new, or an artifact of how the preview is generated.

And the links in the markdown tables at https://json-schema-org-benjagm-fix-3r57.website-2v2.pages.dev/draft/2019-09/release-notes are not rendering still.

@benjagm
Copy link
Collaborator Author

benjagm commented Dec 22, 2024

@DarhkVoyd I spent some hours today trying to fix this without success. I know that the problem is that we need the markdown to be processed recursively for cases like these:

      <td>
        ```json
        {
          "items": [
            { "$ref": "#/$defs/foo" },
            { "$ref": "#/$defs/bar" }
          ]
        }
        ```
      </td>

or

| keyword | change | notes
| ---- | ---- | ----
[`dependentSchemas` (split from `dependencies`)](../../draft/2019-09/json-schema-core.html#rfc.section.9.2.2.4) | **split** | This is the schema form of `dependencies`; note that the standard meta-schema still reserves `dependencies` for backwards compatibility
[`unevaluatedItems`](../../draft/2019-09/json-schema-core.html#rfc.section.9.3.1.3) | **new** | Similar to `additionalItems`, but can "see" into subschemas and across references
[`unevaluatedProperties`](../../draft/2019-09/json-schema-core.html#rfc.section.9.3.2.4) | **new** | Similar to `additionalProperties`, but can "see" into subschemas and across references

We are rendering the top-level element but not what it is inside.

Do you have time to give it a try?

@DarhkVoyd
Copy link
Member

In my opinion, we have correctly configured the Markdown to JSX transpiler. However, the issue lies within the package. The simplest solution here would be to write the Markdown as:

| Keyword                                                                                                                         | Change    | Notes                                                                                                                                         |
| ------------------------------------------------------------------------------------------------------------------------------- | --------- | --------------------------------------------------------------------------------------------------------------------------------------------- |
| <a href="../../draft/2019-09/json-schema-validation.html#rfc.section.6.5.4">`dependentRequired` (split from `dependencies`)</a> | **split** | This is the string array form of `dependencies`; note that the standard meta-schema still reserves `dependencies` for backwards compatibility |
| <a href="../../draft/2019-09/json-schema-validation.html#rfc.section.6.4.4">`maxContains` and `minContains`</a>                 | **new**   | Assertion for controlling how many times a subschema must be matched within an array                                                          |

@benjagm
Copy link
Collaborator Author

benjagm commented Dec 23, 2024

In my opinion, we have correctly configured the Markdown to JSX transpiler. However, the issue lies within the package. The simplest solution here would be to write the Markdown as:

| Keyword                                                                                                                         | Change    | Notes                                                                                                                                         |
| ------------------------------------------------------------------------------------------------------------------------------- | --------- | --------------------------------------------------------------------------------------------------------------------------------------------- |
| <a href="../../draft/2019-09/json-schema-validation.html#rfc.section.6.5.4">`dependentRequired` (split from `dependencies`)</a> | **split** | This is the string array form of `dependencies`; note that the standard meta-schema still reserves `dependencies` for backwards compatibility |
| <a href="../../draft/2019-09/json-schema-validation.html#rfc.section.6.4.4">`maxContains` and `minContains`</a>                 | **new**   | Assertion for controlling how many times a subschema must be matched within an array                                                          |

Thanks for analyzing it DV. While this seems something valid when talking about link inside tables, there are scenarios where we have other stuff like code snippets using the default code block and this is not working as well:

https://json-schema.org/draft/2020-12/release-notes#dollardynamicref-and-dollardynamicanchor

I think what we have is just rendering once, and is not doing the job for other formats inside certain blocks. If we manage to recursively apply formatting we should be able to fix it. I tried this yesterday and managed to make it work for links and other formats, but not code blocks with code snippets.

@DarhkVoyd
Copy link
Member

@benjagm Understood. I'll fix this.

@DarhkVoyd
Copy link
Member

@benjagm I reviewed the library’s source code and the library does parse markdown recursively but the parse logic for table cell content is incorrect. Instead of treating cell content as a single string, it splits it into fragments and loses context, leading to incorrect plain text rendering.

Regarding the code block, it is unrelated as table cells often fail to render properly due to unnecessary line breaks, the fix is simply to remove the unnecessary empty lines between <td> and ```jsonc

For the original table parsing problem, I’ve submitted a detailed issue and suggested a fix for the parsing logic. You can find it here: quantizor/markdown-to-jsx#644

@DarhkVoyd
Copy link
Member

The submitted issue to fix the parsing logic has been attended and a fix has been merged. Should be soon available in a release.

@benjagm
Copy link
Collaborator Author

benjagm commented Jan 7, 2025

The submitted issue to fix the parsing logic has been attended and a fix has been merged. Should be soon available in a release.

Thanks for the update DV!

@benjagm benjagm marked this pull request as draft January 7, 2025 08:23
@DarhkVoyd
Copy link
Member

DarhkVoyd commented Jan 7, 2025

The release with fix is now available as v7.7.3. Let me know if anything else.

@benjagm
Copy link
Collaborator Author

benjagm commented Jan 7, 2025

@DarhkVoyd, Amazing job finding the problem, filing the issue in the markdown-to-jsx dependency, and tracking the problem!! Awesome job.

I just updated the dependency, and the issue with most of the styles has been solved and the only one persisting is the one with the code editor to show code snippets inside cells:

https://json-schema-org-benjagm-fix-3r57.website-2v2.pages.dev/draft/2020-12/release-notes#dollardynamicref-and-dollardynamicanchor

Any idea about what is causing this?

@DarhkVoyd
Copy link
Member

Thanks for the kind words! . Regarding the code editor issue, table cells often fail to render properly due to unnecessary line breaks, the fix is simply to remove the unnecessary empty lines between <td> and ```jsonc just like all other instances.

Before:

Before

After:

After

@benjagm benjagm marked this pull request as ready for review January 7, 2025 13:31
@benjagm
Copy link
Collaborator Author

benjagm commented Jan 7, 2025

Thanks DV. Now everything seems to work. I just re-requested reviews by everyone.

@benjagm benjagm requested review from DarhkVoyd and DhairyaMajmudar and removed request for DarhkVoyd and DhairyaMajmudar January 8, 2025 11:38
Copy link
Member

@DarhkVoyd DarhkVoyd left a comment

Choose a reason for hiding this comment

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

https://json-schema-org-benjagm-fix-3r57.website-2v2.pages.dev/draft/2020-12/release-notes#dollardynamicref-and-dollardynamicanchor still seems to be broken on the build preview but works as desired on the localhost, what am I missing? All else looks good to me.

@DarhkVoyd
Copy link
Member

DarhkVoyd commented Jan 8, 2025

Does this PR predate the PR build preview cache issue fix? Does its preview build still use the outdated cache workflow? Maybe updating this branch with the base main branch will fix this.

Copy link
Member

@DarhkVoyd DarhkVoyd left a comment

Choose a reason for hiding this comment

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

The preview works fine now! Looks good to me, ready to merge!

@benjagm
Copy link
Collaborator Author

benjagm commented Jan 10, 2025

I clicked around a bit more...

This file looks a bit off: https://json-schema-org-benjagm-fix-3r57.website-2v2.pages.dev/draft/2020-12/release-notes (at "$dynamicRef and $dynamicAnchor").

Also selecting the headings results in the underlying html ending up in the copy buffer, which is really weird! but I don't know if that's new, or an artifact of how the preview is generated.

And the links in the markdown tables at https://json-schema-org-benjagm-fix-3r57.website-2v2.pages.dev/draft/2019-09/release-notes are not rendering still.

Now everything is fixed.

@benjagm benjagm requested review from karenetheridge and removed request for karenetheridge January 10, 2025 13:10
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