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

Extract Phlex::Renderable #660

Closed
wants to merge 1 commit into from
Closed

Extract Phlex::Renderable #660

wants to merge 1 commit into from

Conversation

joeldrapper
Copy link
Collaborator

No description provided.

Comment on lines +4 to +6
def call(buffer = +"", context: Phlex::Context.new, view_context:, parent: nil)
raise NoMethodError, "The method `call` must be implemented."
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately there's no way to enforce an abstract method from a module without using TracePoints. So this module just acts to document the interface you must implement.

@joeldrapper joeldrapper marked this pull request as ready for review February 19, 2024 14:31
@joeldrapper
Copy link
Collaborator Author

I'm debating whether content_type and file_name should be part of this interface. 🤔

Comment on lines -225 to +226
when Phlex::SGML
when Phlex::Renderable
Copy link
Collaborator Author

@joeldrapper joeldrapper Feb 19, 2024

Choose a reason for hiding this comment

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

One problem with this is you could potentially render an unsafe CSV into an HTML view. This could be guarded with something like content_type == renderable.content_type.

Copy link
Collaborator Author

@joeldrapper joeldrapper Feb 19, 2024

Choose a reason for hiding this comment

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

Actually, that would prevent you from rendering SVGs inside HTMLs. Perhaps it could be guarded by html_safe?/svg_safe? on the renderable. That way, Phlex::CSV could return false for html_safe?.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could get complicated quickly since each type will essentially be doing some sort of content negotiation.

I'd probably structure this such that a Phlex::CSV has corresponding Phlex::HTML component that's responsible for rendering the Phlex::CSV.

Another example, imagine a Phlex::Console renderer that renders CLI output. What happens if you need to render a Phlex::CSV from Phlex::Console? The Phlex::CSV should have a component that knows how to render itself for a given context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, in any case, I want the ability to define custom classes that claim HTML/SVG/CSV renderability. Not sure how best to do that.

@joeldrapper joeldrapper added this to the 1.10 milestone Feb 20, 2024
@joeldrapper joeldrapper closed this Mar 2, 2024
@joeldrapper joeldrapper deleted the renderable-interface branch March 2, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants