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(table-toolbar): fix console errors when slots are absent #649

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

gavin-everett-genesys
Copy link
Collaborator

Note

The setActionIconOnlyProp() uses the spread operator to copy the array but the spread operator cannot accept null or undefined values which will return an error if the slot does not exist in the DOM. To alleviate this issue I just return an empty array.

✅ Closes: COMUI-3148

Copy link

github-actions bot commented Oct 8, 2024

@@ -139,7 +140,7 @@ export class GuxTableToolbar {
}

get allFilterContextual(): HTMLGuxTableToolbarCustomActionElement[] | null {
return this.filterActions?.concat(this.contextualActions);
return this.filterActions?.concat(this.contextualActions) ?? [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

would this possibly concat a null value making the array at least [null]? Is that caught by ???

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to a more straightforward approach 👍

return [
...(this.filterActions || []),
...(this.contextualActions || [])
].filter(action => action != null);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I should have described this more, but would making the same changes as get permanentActions in get contextualActions and get filterActions be better? That way it removes the null typing entirely and no changes are needed in get allFilterContextual.

Also could turn them into a tertiary depending on how you feel.
i.e.

get permanentActions(): HTMLGuxTableToolbarCustomActionElement[] {
    return this.permanentSlot?.hasChildNodes ?
        Array.from(
             this.permanentSlot?.querySelectorAll(
                 'gux-table-toolbar-action, gux-table-toolbar-custom-action'
             )
       )
       : [];
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah this makes sense to me 👍

fix console errors when slots are absent

✅ Closes: COMUI-3148
@gavin-everett-genesys gavin-everett-genesys merged commit e410f81 into main Oct 9, 2024
3 checks passed
@gavin-everett-genesys gavin-everett-genesys deleted the feature/COMUI-3148 branch October 9, 2024 09:20
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.

2 participants