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(action-menu): update component and add E2E tests #9065

Merged
merged 11 commits into from
Apr 24, 2024

Conversation

alisonailea
Copy link
Contributor

Related Issue: #7180

Summary

Requires #9027 to be merged.

commit 377dc68
Author: Ali Stump <astump@esri.com>
Date:   Wed Apr 3 15:34:57 2024 -0700

    refactor(common): simplify theme tests to be more readable

commit 6b78dfb
Author: Ali Stump <astump@esri.com>
Date:   Mon Apr 1 15:10:11 2024 -0700

    refactor: test utils to simplify assertThemedProps

commit e777ea3
Author: Ali Stump <astump@esri.com>
Date:   Mon Apr 1 12:17:32 2024 -0700

    docs: add jsdocs to common test utils

commit c2244da
Author: Ali Stump <astump@esri.com>
Date:   Mon Apr 1 12:03:21 2024 -0700

    tests: test theme-ing for stateful tokens

commit ec2ec64
Author: Ali Stump <astump@esri.com>
Date:   Wed Mar 27 12:26:56 2024 -0700

    test: allow theme tests to penetrate subcomponent shadow dom

commit 2e0e1d8
Author: Ali Stump <astump@esri.com>
Date:   Tue Mar 26 16:52:00 2024 -0700

    chore: update common theme-ing tests

commit e1d204f
Author: Ali Stump <astump@esri.com>
Date:   Mon Mar 25 15:32:01 2024 -0700

    chore(common): add non-color values for tokenList in themed

commit 2a9472c
Author: Ali Stump <astump@esri.com>
Date:   Mon Mar 25 15:27:59 2024 -0700

    chore(common): add themed common test
…components into astump/7180-fix-action-menu

# Conflicts:
#	packages/calcite-components/src/tests/commonTests.ts
#	packages/calcite-components/src/tests/utils.ts
@alisonailea alisonailea marked this pull request as ready for review April 9, 2024 18:45
@alisonailea alisonailea requested a review from a team as a code owner April 9, 2024 18:45
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

Minor: removal of fragment

@@ -323,9 +323,11 @@ export class ActionMenu implements LoadableComponent {
render(): VNode {
return (
<Fragment>
Copy link
Member

Choose a reason for hiding this comment

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

Since a container is now added then this fragment is no longer necessary. Do we need the container with display:contents?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed to remove <Fragment>, but display:contents is needed to achieve the same layout. <Fragment> doesn't produce an element, so all children are directly under the host when rendered.

},
} as const;
themed(
`<calcite-action-menu open>
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the html tagged template function to help Prettier format embedded HTML?

@alisonailea alisonailea merged commit 30df655 into epic/7180-component-tokens Apr 24, 2024
4 checks passed
@alisonailea alisonailea deleted the astump/7180-fix-action-menu branch April 24, 2024 18:10
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.

3 participants