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

Menubar, Megamenu, Contextmenu and etc.: Twice render if $attrs.id not defined #4953

Closed
i7slegend opened this issue Dec 11, 2023 · 7 comments
Assignees
Labels
Type: Bug Issue contains a bug related to a specific component. Something about the component is not working
Milestone

Comments

@i7slegend
Copy link
Contributor

i7slegend commented Dec 11, 2023

Describe the bug

Components like a menu (Menubar, Megamenu, Contextmenu) and another which generates id in the dom triggers "initial" render twice: first - with id null (if $attrs.id not passed), and next render with generated id

I got example with dirty fix in the codesandbox only with menubar

Reproducer

https://codesandbox.io/p/devbox/frosty-stitch-t2m2m9?file=%2Fsrc%2FApp.vue

PrimeVue version

3.43.0

Vue version

3.x

Language

ALL

Build / Runtime

Vite

Browser(s)

No response

Steps to reproduce the behavior

Create any component which generate id for DOM. All components what do it generate id after mount and trigger render twice also it's triggers submenus if child component uses id too

Expected behavior

Initial render trigger once (ofc if $attrs.id has not changed)

@i7slegend i7slegend added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Dec 11, 2023
@mertsincan mertsincan added Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Jan 16, 2024
@mertsincan mertsincan added this to the 3.47.0 milestone Jan 16, 2024
@mertsincan
Copy link
Member

Fixed in #4954

@mertsincan mertsincan removed this from the 3.47.0 milestone Jan 23, 2024
@mertsincan mertsincan reopened this Jan 23, 2024
@mertsincan mertsincan added Status: Pending Review Issue or pull request is being reviewed by Core Team and removed Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add labels Jan 23, 2024
@mertsincan
Copy link
Member

@i7slegend This PR broke our entire Id structure. Ids are constantly generated as undefined. That's why I reverted it. You can try your PR with Listbox demo;

<ul id="undefined_list" class="p-listbox-list" style="" tabindex="-1" role="listbox" aria-multiselectable="false" aria-disabled="false" data-pc-section="list"> ....

For now, I set a milestone for it. We check it before the milestone is released.
Best Regards,

@mertsincan mertsincan added this to the 3.48.0 milestone Jan 23, 2024
mertsincan added a commit that referenced this issue Jan 23, 2024
@i7slegend
Copy link
Contributor Author

i7slegend commented Jan 23, 2024

@i7slegend This PR broke our entire Id structure. Ids are constantly generated as undefined. That's why I reverted it. You can try your PR with Listbox demo;

<ul id="undefined_list" class="p-listbox-list" style="" tabindex="-1" role="listbox" aria-multiselectable="false" aria-disabled="false" data-pc-section="list"> ....

For now, I set a milestone for it. We check it before the milestone is released. Best Regards,

Got it.
Yeah it's reproduce under the nuxt server. In "standalone" vue components it's works correctly.

So i have test nuxt variant and see what beforeMount() not called on the server.

I test the case with data() & beforeCreate() and found what it's called at same time (very very close) and it's affect to next problem: data() replace id to undefined after beforeCreate() hook.
IMHO, in this case enough move initial UniqueComponentId to the data() instead use in any hooks (at least under the nuxt framework). Anyway it's only my theory 🙃

@i7slegend
Copy link
Contributor Author

Attach code block (on example Listbox) which works under nuxt as expected:

    data() {
        return {
            id: this.$attrs.id || UniqueComponentId(),
            filterValue: null,
            focused: false,
            focusedOptionIndex: -1
        };
    },
    watch: {
        '$attrs.id': function (newValue) {
            this.id = newValue || UniqueComponentId();
        },
        options() {
            this.autoUpdateModel();
        }
    },
    mounted() {
        this.autoUpdateModel();
    },

@tugcekucukoglu tugcekucukoglu added Type: Bug Issue contains a bug related to a specific component. Something about the component is not working and removed Status: Pending Review Issue or pull request is being reviewed by Core Team labels Jan 26, 2024
@tugcekucukoglu tugcekucukoglu self-assigned this Jan 26, 2024
@mertsincan
Copy link
Member

Hi @i7slegend,

Thanks a lot for the detailed info! Yes, we are working to update it like in the above code block.

@mertsincan mertsincan modified the milestones: 3.48.0, 3.49.0 Feb 2, 2024
@mertsincan mertsincan self-assigned this Feb 2, 2024
mertsincan added a commit that referenced this issue Feb 2, 2024
mertsincan added a commit that referenced this issue Feb 2, 2024
mertsincan added a commit that referenced this issue Feb 2, 2024
mertsincan added a commit that referenced this issue Feb 2, 2024
mertsincan added a commit that referenced this issue Feb 2, 2024
mertsincan added a commit that referenced this issue Feb 2, 2024
mertsincan added a commit that referenced this issue Feb 2, 2024
mertsincan added a commit that referenced this issue Feb 2, 2024
mertsincan added a commit that referenced this issue Feb 2, 2024
mertsincan added a commit that referenced this issue Feb 2, 2024
mertsincan added a commit that referenced this issue Feb 2, 2024
@i7slegend
Copy link
Contributor Author

i7slegend commented Feb 29, 2024

Hello! Sorry for reopen this issue, i found what commits above doesn't goals to fix trigger render twice in this issue's example. It's reproduces now

I mean in the attached code block before (#4953 (comment)) this (use OR operator in state component for id property).
Example for Menubar component:

    data() {
        return {
-           id: this.$attrs.id,
+           id: this.$attrs.id || UniqueComponentId(),
            filterValue: null,
            focused: false,
            focusedOptionIndex: -1
        };
    },
    watch: {
        '$attrs.id': function (newValue) {
            this.id = newValue || UniqueComponentId();
        },
        options() {
            this.autoUpdateModel();
        }
    },
    mounted() {
-       this.id = this.id || UniqueComponentId();
        this.autoUpdateModel();
    },

@i7slegend
Copy link
Contributor Author

i7slegend commented Feb 29, 2024

Oh, issue not reopened 😖

@mertsincan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issue contains a bug related to a specific component. Something about the component is not working
Projects
None yet
Development

No branches or pull requests

3 participants