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

feat request for posthtml-mso #1

Open
Scrum opened this issue Apr 27, 2020 · 19 comments
Open

feat request for posthtml-mso #1

Scrum opened this issue Apr 27, 2020 · 19 comments

Comments

@Scrum
Copy link
Member

Scrum commented Apr 27, 2020

Need to use it in posthtml-mso, so this:

<outlook><table><tr><td></outlook>

results in this:

<!--[if mso|ie]><table><tr><td><![endif]-->

and not in this:

<!--[if mso|ie]><table><tr><td></td></tr></table><![endif]-->
@cossssmin
Copy link
Member

Additional context:

It's common when coding HTML emails to use 'ghost tables' for Outlook - this is markup written inside conditional comments that target Outlook on Windows, so that it gets used only in that email client.

The most frequent usage is to define fixed-width tables for Outlook when coding mobile-first emails - a technique also known as 'hybrid' in email coding.

The problem with the parser right now is that it automatically closes any unclosed tag it encounters, making it impossible to use posthtml-mso for hybrid emails.

Basically this:

<outlook><table><tr><td></outlook>
  <div>Example</div>
<outlook></td></tr></table></outlook>

Is output like this:

<!--[if mso|ie]><table><tr><td></td></tr></table><![endif]-->
    <div>Example</div>

Notice also how </td></tr></table> doesn't make it in the final output (empty last line).

So the correct output would actually be:

<!--[if mso|ie]><table><tr><td><![endif]-->
    <div>Example</div>
<!--[if mso|ie]></td></tr></table><![endif]-->

@anikethsaha
Copy link
Member

I think this proposal is good. But I strongly belive that the core posthtml libraries (posthtml, posthtml-parser and posthtml-renderer) should follow the specs as much as possible. also I am not familiar with the mso thing . is it a standard thing for html ? (I think its not right ?)

I would prefer to something like this in posthtml-mso instead of here cause there might be other plugins which needs custom HTML structure and posthtml 's renderer can be extended to plugins. so I would prefer that.

let me know. I may miss something.

@cossssmin
Copy link
Member

should follow the specs as much as possible

Agree 100%. Not sure how this would be done exactly, but basically what this needs is the unparsed contents of a tag that PostHTML touches - makes sense?

@anikethsaha
Copy link
Member

anikethsaha commented Apr 27, 2020

but basically what this needs is the unparsed contents of a tag that PostHTML touches - makes sense?

still a bit lacking. can you give some more details. ? you meant those thing which are not in AST?

@Scrum
Copy link
Member Author

Scrum commented Apr 27, 2020

I would prefer to something like this in posthtml-mso instead of here cause there might be other plugins which needs custom HTML structure and posthtml 's renderer can be extended to plugins. so I would prefer that.

At the moment, I am of the same opinion since something similar (Downlevel-revealed because the parser does not handle correctly) I had to implement in posthtml-beautify

https://github.com/posthtml/posthtml-beautify/blob/0d60579f22c16d1baf80ae609d81a879a2e9a696/src/index.js#L55-L92

@Scrum
Copy link
Member Author

Scrum commented Apr 27, 2020

For now, move it to posthtml-mso

@Scrum Scrum transferred this issue from posthtml/posthtml-render Apr 27, 2020
@anikethsaha
Copy link
Member

@Scrum can we support officially a way to override the renderer like it is done for parser ?

@cossssmin it might be an overhead to implement here instead of posthtml-render. let me know if you need any hand 👍

@Scrum
Copy link
Member Author

Scrum commented Apr 27, 2020

@anikethsaha

can we support officially a way to override the renderer like it is done for parser ?

I don't see any reason why we shouldn't do this, and what do you think is missing in the current render ?

@anikethsaha
Copy link
Member

and what do you think is missing in the current render ?

as of now, I couldn't see anything missing. but for cases like this issue, it would be better to have an option. just to increase flexibility. developers can write their own renderer from scratch if they need or they can extend the current one which is done by posthtml-beautify and mentioned in the code
here.

@Scrum
Copy link
Member Author

Scrum commented Apr 27, 2020

The decisions made in posthtml-beautify were caused by the fact that the parser didn’t process correctly Downlevel-revealed, I have a solution that processes correctly Downlevel-revealed and many other problems in the new @posthtml/parse but I still can not get together and add it))

But I like the idea of allowing users to set their parser or be able to expand the current

@Scrum
Copy link
Member Author

Scrum commented Apr 29, 2020

Thoughts for implementation

  1. First of all, it is necessary to designate the opening and closing tag

from

<outlook><div></outlook>

to

<outlook type="open"><div><outlook type="close">

thereby we get a tree

[
  {
    tag: "outlook",
    attrs: {type="open"},
    content: [
      {
        tag: "div",
        content: [
          {
            tag: "outlook",
            attrs: {type="close"}
          }
        ]
      }
    ]
  }
]
  1. mutate the tree in
[
  {
    tag: false,
    attrs: {},
    content: [
      '<!--[if mso|ie]>',
      {
        tag: "div",
        content: [
          {
            tag: false,
            attrs: {},
            content: [
              '<![endif]-->'
            ]
          }
        ]
      }
    ]
  }
]
  1. accordingly, after rendering
<!--[if mso|ie]><div><![endif]--></div>

As far as you remember, there is now access to tree.source You can take the source, make the necessary transformations in it, and it’s best to do this in advance, apply all the plugins that you can take tree.plugins apply to the transformed source and then apply your method to the resulting tree

@cossssmin
Copy link
Member

I would definitely not want to force the user into defining opening/closing tags through type attributes. This is an implementation detail that they shouldn’t have to care about.

At point 3) the desired output would be

<!--[if mso|ie]><div><![endif]-->

But I suppose I can remove everything after the closing endif 👍

@Scrum
Copy link
Member Author

Scrum commented Apr 29, 2020

I would definitely not want to force the user into defining opening/closing tags through type attributes. This is an implementation detail that they shouldn’t have to care about.

The notation remains the same for the user, and all the magic described above happens inside.

@Scrum
Copy link
Member Author

Scrum commented May 8, 2020

@cossssmin All attempts were in vain, the absence of a closing tag takes the tree deeper, its presence makes all the content content.

@Scrum
Copy link
Member Author

Scrum commented Mar 9, 2021

@cossssmin Hi, what was the reason to close this issue?

@cossssmin
Copy link
Member

Based on your previous comment - if we can still tackle this, my bad 😬

@cossssmin cossssmin reopened this Mar 9, 2021
@Scrum
Copy link
Member Author

Scrum commented Mar 9, 2021

Based on your previous comment - if we can still tackle this, my bad 😬

We can, I will try to allocate time for this.

@Scrum
Copy link
Member Author

Scrum commented May 27, 2021

@cossssmin Hi, I think it's worth trying the new option using for posthtml new version of render with option closeas

@cossssmin
Copy link
Member

Getting back to this after a while, this seems to do it for opening tags:

node.content = `<!--[if mso]>${tree.render(node.content, {
        singleTags: ['table', 'tr', 'td'],
      })}<![endif]-->`

      return node

... however it doesn't work on something like this because there is no node.content:

<outlook>
  </td></tr></table>
</outlook>

Not sure what can be done here, @Scrum any ideas?

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

No branches or pull requests

3 participants