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

fix: menu list as template #163

Merged
merged 3 commits into from
Jan 16, 2024
Merged

Conversation

dword-design
Copy link

I turned the menu list into a template, first for consistency and the function implementation didn't work in Nuxt production.

@kikuomax kikuomax marked this pull request as draft January 7, 2024 07:08
@kikuomax
Copy link
Collaborator

kikuomax commented Jan 7, 2024

@dword-design Thank you for your contribution! But could you make the PR to dev branch?

@dword-design dword-design changed the base branch from main to dev January 8, 2024 08:06
@dword-design
Copy link
Author

@kikuomax done

wesdevpro
wesdevpro previously approved these changes Jan 8, 2024
Copy link

@wesdevpro wesdevpro left a comment

Choose a reason for hiding this comment

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

LGTM⚡Please wait for @kikuomax to review this code before pulling this PR into dev.
(I just realized this PR is still a draft 😅)

@kikuomax
Copy link
Collaborator

kikuomax commented Jan 9, 2024

@dword-design @wesdevpro Please give me some time to see if there might be potential drawbacks.

@kikuomax
Copy link
Collaborator

kikuomax commented Jan 9, 2024

@dword-design @wesdevpro I will look into this PR after I finish #159.

@kikuomax
Copy link
Collaborator

I turned the menu list into a template, first for consistency and the function implementation didn't work in Nuxt production.

@dword-design Did you mean functional components wouldn't work in Nuxt production in general?

@kikuomax kikuomax marked this pull request as ready for review January 15, 2024 09:05
@dword-design
Copy link
Author

dword-design commented Jan 15, 2024

I turned the menu list into a template, first for consistency and the function implementation didn't work in Nuxt production.

@dword-design Did you mean functional components wouldn't work in Nuxt production in general?

@kikuomax Ok I debugged myself into this and I think the problem is that we use component.name to register the components in the plugins. This is generally not a problem since also for functional components component.name will be the name of the function (in this case BMenuList). But in production and in the browser the function name will be a generated one (in this case ng), so the component name is wrong. In other cases we explicitly set the name as a fiel via Options API. So functional components aren't directly a problem in Nuxt but it's more how it's solved in Buefy.

@kikuomax
Copy link
Collaborator

@dword-design Great insight!

Now, I remember I faced the exact problem fixed at d44e6d2 that is not merged to main branch yet.

@kikuomax
Copy link
Collaborator

@dword-design Great insight!

Now, I remember I faced the exact problem fixed at d44e6d2 that is not merged to main branch yet.

While the problem should have already been solved, I think an ordinary component is more intuitive and this PR is still relevant.

Copy link
Collaborator

@kikuomax kikuomax left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@kikuomax kikuomax merged commit decc2c0 into ntohq:dev Jan 16, 2024
3 checks passed
@dword-design dword-design deleted the menu-list-template branch January 16, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants