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

Added markdown extensions support #14419

Open
wants to merge 9 commits into
base: 4.x
Choose a base branch
from
Open

Added markdown extensions support #14419

wants to merge 9 commits into from

Conversation

Plytas
Copy link
Contributor

@Plytas Plytas commented Oct 2, 2024

Description

As proposed in #1294 (comment), this PR adds support for markdown extensions.

One important change - HTML is sanitized before being passed to markdown() function. This still sanitizes user input, but allows markdown to produce "unsafe" HTML like <iframe>.

I'm starting this as draft PR to receive feedback. I'll be able to update docs once desired API is confirmed. Also, let me know if this needs tests and point me to an example I can follow.

Visual changes

No visual changes.

Functional changes

  • Code style has been fixed by running the composer cs command.
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

@Plytas
Copy link
Contributor Author

Plytas commented Oct 2, 2024

Ping @danharrin @ryangjchandler.

@danharrin danharrin added this to the v4 milestone Oct 2, 2024
Copy link
Member

@danharrin danharrin left a comment

Choose a reason for hiding this comment

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

I like the idea! Few things:

  • I think all the properties and methods should be renamed to commonMarkOptions() and commonMarkExtensions() for clarity
  • I think it should apply to the TextColumn and TextEntry, "summarizer" classes too, which can render Markdown using the markdown() method
  • Since we're using it in multiple places, extract the properties and methods to a trait CanConfigureCommonMark in Filament\Support\Concerns

@Plytas
Copy link
Contributor Author

Plytas commented Dec 4, 2024

@danharrin I have updated the PR with requested changes.

The failing actions are for Laravel 10. Str::markdown() does not accept extensions as it was introduced in v11.13.0. Is there a good way to check if extension support is available and only then pass them in?

@Plytas Plytas marked this pull request as ready for review December 4, 2024 16:20
@Plytas Plytas requested a review from danharrin December 4, 2024 16:20
@danharrin
Copy link
Member

Laravel 10 will soon be out of security release support, so I will probably drop it in v4 anyway. I will update this PR when I do so.

Thanks for the rest of it, it looks perfect!

@Plytas
Copy link
Contributor Author

Plytas commented Dec 5, 2024

You might have to lock it to at least v11.13.0 then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants