-
Notifications
You must be signed in to change notification settings - Fork 4
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
improve: LDP-2575: Add renderCE fallback to lookup for default components #255
Conversation
const formatName = (name: string) => name.split('-').map(part => part.charAt(0).toUpperCase() + part.slice(1)).join('') | ||
|
||
// Try resolving the full component name. | ||
const component = nuxtApp.vueApp.component(formatName(element)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using nuxtApp.vueApp.component instead of Vue's resolveComponent because it returns a warning for every attempt at resolving a custom element, even if it exists as a Default component(so a lot of warnings in browser + server console if it's multiple CEs)
if not ok I can revert back to use Vue's resolveComponent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, makes sense
*/ | ||
const resolveCustomElement = (element: string) => { | ||
const nuxtApp = useNuxtApp() | ||
const formatName = (name: string) => name.split('-').map(part => part.charAt(0).toUpperCase() + part.slice(1)).join('') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: But that kebap-case to camelCase processing is done already by Vue automatically, so I think we don't need to do it, right? If so, let's better keep our code as simple as possible and let Vue handle that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nuxtApp.vueApp.component only resolves PascalCase
const formatName = (name: string) => name.split('-').map(part => part.charAt(0).toUpperCase() + part.slice(1)).join('') | ||
|
||
// Try resolving the full component name. | ||
const component = nuxtApp.vueApp.component(formatName(element)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, makes sense
? h('div', customElements.map(customElement => h(resolveComponent(customElement.element), customElement))) | ||
: h(resolveComponent(customElements.element), customElements) | ||
if (Array.isArray(customElements)) { | ||
return h('div', customElements.map((customElement) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we actually require this wrapping div? if code-wise this is not needed, I think it would be better to not have it. We could make the helper return an array of elements always if that makes using it easier.
closes #250 |
}) | ||
it('missing node element is rendered as node--default', async () => { | ||
const html = await $fetch('/node/4') | ||
expect(html).toContain('This node is testing custom elements fallback. node-article-demo --> node--default') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not really a good test:
a) we have no proof node--default is really used, it cuold be any template. would be good to check for something what is in there, but not passed as content.
b) we should verify that when node-article-demo.vue is added this is used instead alsl
} | ||
|
||
// Try resolving by adding 'Default' suffix. | ||
const defaultComponent = nuxtApp.vueApp.component(formatName(element) + 'Default') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to do this when it's done above? should it just be a different loop, do-while ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is different because the while loop above checks for - symbol, e.g. node-demo, so a simple "node" will not trigger the while loop
i've refactored the regex and loop to first try adding default then remove segments
No description provided.