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.

Sign up for GitHub

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

Jump to bottom

feat: Supply HTML Template for Playroom & Preview #189

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

feat: Supply HTML Template for Playroom & Preview #189

wants to merge 3 commits into from

Conversation

alexbchr
Copy link
Contributor

Adds new htmlTemplate and previewHtmlTemplate options in config, which receive a path to the HTML template used to render Playroom (and Preview).

Sample usage:

{
  // ...
  htmlTemplate: './template.html',
  previewHtmlTemplate: './preview-template.html',
  // ...
}

If no template is provided, Playroom renders exactly as it did before this change.

Note: Template gets loaded by HtmlWebpackPlugin as mentioned in Readme. This way, we can leverage all the power of loaders or simply supply a simple .html file.

I also extended themed example with the new options, for example usage.

This will also fix issue #40.

@alexbchr alexbchr requested a review from a team as a code owner September 14, 2020 14:41
@alexbchr alexbchr mentioned this pull request Sep 14, 2020
Closed
@michaeltaranto
Copy link
Contributor

This looks a lot more promising. Seeing this implemented raises a few questions around the ambiguity between the rendering of frames versus the whole document. For example if you need a font, or css from a CDN etc, these should be setup via the FrameComponent as part of creating the environment for your app to render into.

With that in mind, if we are thinking about reasons to access the top level document, a few questions come up:

  • Are we just talking about analytics scripts?
  • What is the need to differentiate between previewTemplate and htmlTemplate?
  • What happens to config options like title that were previously setting the page title?

@alexbchr
Copy link
Contributor Author

This looks a lot more promising. Seeing this implemented raises a few questions around the ambiguity between the rendering of frames versus the whole document. For example if you need a font, or css from a CDN etc, these should be setup via the FrameComponent as part of creating the environment for your app to render into.

To clarify the distinction between FrameComponent, Playroom HTML and Preview HTML, maybe there could be a section about this in Readme, or simply a comment or clarification in Readme. I think this can be overcome with some clarification in the Readme. We could also include these options in some "Advanced" section in the Readme, so people understand the real implications of editing these options.

With that in mind, if we are thinking about reasons to access the top level document, a few questions come up:

  • Are we just talking about analytics scripts?

No, here are other use cases I have and are solved by the HTML template:

  • Custom Favicon
  • Custom Metatags (OG tags, etc.)
  • Custom Page Title (with different format that the provided option)
  • Watermark (absolutely positioned div in HTML content) or wrapper around Playroom.
  • What is the need to differentiate between previewTemplate and htmlTemplate?

For me, these pages are highly different, so I needed to have distinct elements on the 2 pages:

  • Different Metatags
  • Different Page Title (with different format that the provided option)
  • Don't have watermark like Playroom.

I preferred to separate both options for customizability. Should someone ever need the exact same layout for both, it can easily be done using the same file for both options.

  • What happens to config options like title that were previously setting the page title?

2 possible outcomes here with current implementation:

  1. If specifying a .html file, the title config option will be completely ignored.
  2. If specifying a .ejs file, the title config option behavior can be preserved by using EJS template tags like in the default HtmlWebpackPlugin template. Here is the template for reference:
<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title><%= htmlWebpackPlugin.options.title %></title>
  </head>
  <body>
  </body>
</html>

@michaeltaranto
Copy link
Contributor

Yeah gotcha. I tend to agree that documentation is probably the only defence here. In terms of configuration changes, what are your thoughts on htmlTemplate being an object with keys for the routes? Eg:

{
  htmlTemplate: {
    '/': './pathToMainTemplate.ejs',
    '/preview': './pathToPreviewTemplate.ejs',
  }
}

May be more future proof and break the one-to-one route to template configuration.

@alexbchr
Copy link
Contributor Author

Yeah gotcha. I tend to agree that documentation is probably the only defence here. In terms of configuration changes, what are your thoughts on htmlTemplate being an object with keys for the routes? Eg:

{
  htmlTemplate: {
    '/': './pathToMainTemplate.ejs',
    '/preview': './pathToPreviewTemplate.ejs',
  }
}

May be more future proof and break the one-to-one route to template configuration.

I like the idea of nesting the whole template configuration in a nested object. In fact this was how I did it upfront but since all other configuration don't have nested keys (from what I remember), I went with the "flat" way.

What I first thought was going on with arbitrary keys, like this:

{
  htmlTemplate: {
    'home': './pathToMainTemplate.ejs',
    'preview': './pathToPreviewTemplate.ejs',
  }
}

However I dislike the term home and if using playroom, I found this config tree a bit confusing.

After much thought, I like the idea of using the path, but I feel this will need to be documented a bit more, as I fear this may cause confusion if used in conjunction with the baseUrl config option.

@alexbchr
Copy link
Contributor Author

@michaeltaranto Updated code to use new nested configuration with paths (paths act as constants, template value is not inferred dynamically from current path).

Also updated documentation to reflect previous discussions.

Also, just a comment here! When I started playing with Playroom, I found options quite self-explanatory and simple. However, some more advanced options, such as this one or iframeSandbox, pretty much need explanations of their own. It is also not that clear if the example config in the Readme contains all possible config options or just a few.

I think it would be very useful (especially for newcomers), to have all options documented somewhere (in the Readme or a separate page) (in a table or something). Maybe only options that are considered as "advanced" would need such treatment, but I think it would still gain in clarity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexbchr @michaeltaranto