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: parse vue components in text elements #744

Merged
merged 13 commits into from
May 29, 2020

Conversation

Rigo-m
Copy link
Collaborator

@Rigo-m Rigo-m commented May 14, 2020

Changes

closes #649 - Refactor CmsElementText.vue to have a render function that maps certain html elements to vue components

Checklist

@vercel
Copy link

vercel bot commented May 14, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/shopware-pwa/shopware-pwa-docs/5hgtpvs10
✅ Preview: https://shopware-pwa-doc-git-fork-rigo-m-feat-calls-to-action-pa-2ec8c8.shopware-pwa.now.sh

@CLAassistant
Copy link

CLAassistant commented May 14, 2020

CLA assistant check
All committers have signed the CLA.

@vercel vercel bot temporarily deployed to preview May 14, 2020 20:39 Inactive
@Rigo-m
Copy link
Collaborator Author

Rigo-m commented May 14, 2020

A couple of words on the code:
It's still a WIP, I've pushed so you can discuss the work done and give me some feedback.
What has been done:

  • HTML to AST to Vue render function
  • Vue render function skip compiling if there is no element to compile
  • We can remove/add components with a map that defines both the condition that makes the component replace a specific HTML tag and the render function that goes with it (see example with SfLink)
    What's missing:
  • Boring code implementations should be invisible (most of the functions like the renderer, the AST visit function etc should be moved in another location
  • variables naming seems a bit off to me but maybe it's ok
  • extraComponentsMap should be moved somewhere else in a dedicated spot
  • Implement the correct number of extraComponents with every possibility (buttons, links, secondary and primary etc)

I hope I've done an acceptable job, if it seems alright I'll proceeds with what's missing (need some help on where to put code implementation and extraComponentsMap. Also, I need some help understanding how many components I should map and to which .vue file)

@pkarw
Copy link

pkarw commented May 18, 2020

This is great feature however I'm a little bit worried about the bundle size @patzick. Maybe we should load the parser dynamically just when it's really needed. The second idea is that maybe we should have this logic in kind-of a server-side service (filtering out the CMS content before it landed from the API - to streamline the client's side logic)

@Rigo-m
Copy link
Collaborator Author

Rigo-m commented May 18, 2020

Hi Piotr, bundle size is not an issue IMHO, the parser I've used is 600 bytes minified+gzipped. It's so small because it's based on regexes and assumes the HTML is valid XHTML (each tag is closed correctly).

Generating the AST server side, therefore, doesn't change that much

@vercel vercel bot temporarily deployed to preview May 18, 2020 18:18 Inactive
@patzick
Copy link
Collaborator

patzick commented May 18, 2020

good thing this component is already lazy-loaded only when CMS text-element is used

but @Rigo-m as html-parse-stringify2 is very light, other two lodash and html-entities are unfortunately too large to be used here, look at this:

so it's almost 40kB for a single component. Do you see a way to use it without these two libs?

The rest of it looks very promising, I'll test it shortly and dive deep into code :)

@Rigo-m
Copy link
Collaborator Author

Rigo-m commented May 18, 2020

Lodash can be cherry picked https://bundlephobia.com/result?p=lodash.isequal@4.5.0 (I'll do it while polishing the code), I'll find a solution for html-entities for sure, thanks for pointing that out.

My goal right now is to publish a small library to npm to convert html to render functions, so that code implementation will be hidden behind the library (and also other people will be able to use it)

I'll see if I can manage to implement @pkarw advices about lazy loading imports as well

The end result will be something like

export default {
  render: function (createElement) { 
  return renderer(this.rawHtml, config, createElement)
}
[...]
}

Sounds good to you?

@vercel vercel bot temporarily deployed to preview May 27, 2020 16:40 Inactive
@vercel vercel bot temporarily deployed to preview May 28, 2020 10:17 Inactive
@vercel vercel bot temporarily deployed to preview May 28, 2020 11:31 Inactive
@vercel vercel bot temporarily deployed to preview May 28, 2020 23:37 Inactive
@vercel vercel bot temporarily deployed to preview May 28, 2020 23:50 Inactive
@vercel vercel bot temporarily deployed to preview May 29, 2020 10:14 Inactive
Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

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

This is 🔥

@patzick patzick merged commit 3b6b5ac into vuestorefront:master May 29, 2020
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.

[FEATURE] Calls to Action inside text elements should be parsed
4 participants