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

Add option to preserve whitespace for certain elements #154

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

Conversation

sventschui
Copy link
Member

Add an option to preserve whitespace/indentation for certain elements. Defaults to ['pre']

Caution I'm not 100% sure this doesn't break pretty printing, tests seem fine though.

Fixes #143

Add an option to preserve whitespace/indentation for certain elements. Defaults to `['pre']`

Signed-off-by: Sven Tschui <[email protected]>
src/index.js Outdated
@@ -73,7 +74,7 @@ function renderToString(vnode, context, opts, inner, isSvgMode, selectValue) {
for (let i = 0; i < children.length; i++) {
rendered += (i > 0 && pretty ? '\n' : '') + renderToString(children[i], context, opts, opts.shallowHighOrder!==false, isSvgMode, selectValue);
}
return rendered;
return pretty ? indent(rendered, indentChar) : rendered;
Copy link
Member Author

@sventschui sventschui Jun 2, 2020

Choose a reason for hiding this comment

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

Had to move the call to indent here to not indent HTML content but do indent Fragment children. I'm not 100% sure this doesn't break other things. I'll try to add some more tests.

EDIT yes indeed it breaks text indentation :(

src/index.js Outdated
@@ -20,6 +20,7 @@ const noop = () => {};
* @param {Boolean} [options.shallow=false] If `true`, renders nested Components as HTML elements (`<Foo a="b" />`).
* @param {Boolean} [options.xml=false] If `true`, uses self-closing tags for elements without children.
* @param {Boolean} [options.pretty=false] If `true`, adds whitespace for readability
* @param {Boolean} [options.preserveWhitespace=['pre']] Array of HTML tags for which to preserve indentation.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: type should be string array here 👍

@sventschui
Copy link
Member Author

It seems the change will break indentation of text nodes. Honestly I'm not sure we can actually change text indent in a safe way. I'd therefore suggest to not indent html (dangerouslySetInnerHtml, string children, etc.) in pretty mode

@sventschui
Copy link
Member Author

Just to keep you posted: I'll add the discussion with @marvinhagemeister on slack here publicly:

Marvin mentioned the issue is that we are indenting after rendering instead of during rendering. While trying to change that I ran into issues on how to determine whether we need to wrap/indent text content/children.

@natemoo-re
Copy link

This is unfortunately a pretty big issue for microsite, a Preact-based SSG, since developer blogs with code examples are a big use case. I'd like to keep pretty enabled in the spirit of the "view source" web.

Any updates?

@developit
Copy link
Member

@natemoo-re Are you using the { pretty: true } option? It's really only designed for use in things like tests - none of the options for renderToString() are part of the public API.

@natemoo-re
Copy link

@developit I was at the time! We've since moved to formatting the output as a separate step for other reasons.

@developit
Copy link
Member

@natemoo-re ah okay, good. I'd like to remove this from the default render to string implementation.

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.

Pretty rendering breaks <pre> tags
4 participants