jahilldev / component-elements

Create a custom element from any component with these tiny functions (2KB GZipped, ~1KB Brotli). Preact and React currently supported
MIT License
89 stars 7 forks source link

Whitespace being trimmed when rendering children #28

Closed freshly-pressed-trousers closed 1 year ago

freshly-pressed-trousers commented 1 year ago

We've been using this with some great success for a while now but have come across an issue where whitespace seems to be trimmed in places when run through it. I've managed to replicate it fairly minimally.

The issue manifests itself like this: image

The component itself is just:

import { define } from 'preactement';
import PropTypes from 'prop-types';

export default function TestComponent({ children }) {
    return children;
}

TestComponent.propTypes = {
    children: PropTypes.node.isRequired,
}

TestComponent.tagName = 'test-component'
TestComponent.attributes = [];

define(TestComponent.tagName, () => TestComponent, { attributes: TestComponent.attributes });

Before the JS is run the page is provided to the browser as:

image

And once it kicks in it seems to have a cheeky nibble at some of the whitespace:

image

freshly-pressed-trousers commented 1 year ago

https://github.com/jahilldev/component-elements/blob/main/packages/preactement/src/parse.ts#L36 looks very suspicious

jahilldev commented 1 year ago

Hey @freshly-pressed-trousers (always important...),

Thanks for reaching out. I'm trying to remember why I did this initially, there was a reason...

Feel free to open a PR and test removing the .trim() call, that seems like a likely candidate. I'll try and take a proper look when I get time, but this looks like a perfect first issue for you to take a bash at!

Also, thanks and appreciate the upfront repro, makes life so much easier. If you could share the repo with your reproduction that would be super helpful.

All the best 👍

freshly-pressed-trousers commented 1 year ago

I'll have a crack at removing the trim this weekend.

No doubt it's probably there for a reason, might be some ambiguity between structural whitespace and content whitespace which I presume most JSX transpilers handle inside of components but when taking html content off the dom might be a tad trickier.

freshly-pressed-trousers commented 1 year ago

Removing the trim seems to have fixed it, haven't tested many other cases but the immediate issue is resolved. Will look at the impact on the rest of our components.

We're looking to build an accordion component using this that will need to retain the whitespace of quite a wide variety of nested content (text, media, paragraphs, lists, etc), so will probably use that as an opportunity to see if anything else crawls out the woodwork and if all looks good will raise a PR.

jahilldev commented 1 year ago

Hey @freshly-pressed-trousers,

How's the testing going? If you're ready, feel free to open a PR and we can get it merged / published 👍

freshly-pressed-trousers commented 1 year ago

Hey sorry for the delay, PR is here - https://github.com/jahilldev/component-elements/pull/29

I can see there's trim() calls in the other integrations, however I can't test them with my current setup so I've left them alone.

freshly-pressed-trousers commented 1 year ago

Do you mind running a publish to push the recent version up to npm, thanks 🙏

jahilldev commented 1 year ago

Hey @freshly-pressed-trousers,

Sorry, the CD action has broken somehow here, I'll need to fix when I get a sec.

For now, I've manually published v1.8.5 of preactement with your changes 👍

Thanks again for the contribution!