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.

Sign up for GitHub

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

Jump to bottom

feat(action-menu, block-section, list-item, menu-item): add open, close, beforeOpen, and beforeClose events for consistent event handling #9736

Open
wants to merge 21 commits into
base: dev
Choose a base branch
from
Open

feat(action-menu, block-section, list-item, menu-item): add open, close, beforeOpen, and beforeClose events for consistent event handling #9736

wants to merge 21 commits into from
+273 −43

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Jul 4, 2024 •
edited
Loading

Related Issue: #6017, #4544

Summary

Implement OpenCloseComponent with associated events to action-menu, block-section, list-item, menu-item.

@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Jul 4, 2024
@Elijbet Elijbet changed the title feat(action-menu): implement OpenCloseComponent interface feat(action-menu, block-section): implement OpenCloseComponent interface Jul 4, 2024
@Elijbet Elijbet changed the title feat(action-menu, block-section): implement OpenCloseComponent interface feat(action-menu, block-section, list-item): implement OpenCloseComponent interface Jul 4, 2024
@Elijbet Elijbet changed the title feat(action-menu, block-section, list-item): implement OpenCloseComponent interface feat(action-menu, block-section, list-item, menu-item): implement OpenCloseComponent interface Jul 5, 2024
@Elijbet Elijbet marked this pull request as ready for review July 12, 2024 22:36
@Elijbet Elijbet requested a review from a team as a code owner July 12, 2024 22:36
@Elijbet Elijbet requested review from driskull and jcfranco July 12, 2024 22:36
@Elijbet Elijbet changed the title feat(action-menu, block-section, list-item, menu-item): implement OpenCloseComponent interface feat(action-menu, block-section, list-item, menu-item): implement OpenCloseComponent interface Jul 14, 2024
@Elijbet Elijbet changed the title feat(action-menu, block-section, list-item, menu-item): implement OpenCloseComponent interface feat(action-menu, block-section, list-item, menu-item): adds open, close, beforeOpen, and beforeClose events for consistent event handling Jul 16, 2024
@Elijbet Elijbet changed the title feat(action-menu, block-section, list-item, menu-item): adds open, close, beforeOpen, and beforeClose events for consistent event handling feat(action-menu, block-section, list-item, menu-item): add open, close, beforeOpen, and beforeClose events for consistent event handling Jul 16, 2024
@benelan benelan removed the request for review from a team July 16, 2024 23:47
@alisonailea
Copy link
Contributor

alisonailea commented Jul 17, 2024 •
edited
Loading

I'd like to suggest we normalize all components to a single "Closable" class that includes these evens as well as the template for a consistent close button UI that can be added to the JSX. @jcfranco

@jcfranco
Copy link
Member

@alisonailea Let's discuss that separately since there are a few challenges with your suggestion. Namely,

  • Stencil doesn't support mixins nor extending classes.
  • Events are typed and based on the component name.
  • The close button is not entirely tied to these events.

driskull
driskull requested changes Jul 17, 2024
View reviewed changes
packages/calcite-components/src/components/menu-item/menu-item.tsx
@@ -465,6 +513,7 @@ export class CalciteMenuItem implements LoadableComponent, T9nComponent, Localiz
[CSS.container]: true,
[CSS.isParentVertical]: this.topLevelMenuLayout === "vertical",
}}
ref={this.setTransitionEl}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this overwrite the other transitionEl set on line 472?

Copy link
Contributor Author

@Elijbet Elijbet Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not exactly sure where the right place for it is. It's kind of a Russian doll situation. When you slot items into menu-items it renders menu. So you could set it on the container for menu-item, or menu itself?

driskull
driskull approved these changes Jul 24, 2024
View reviewed changes
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.

LGTM! Lets wait for @jcfranco to review.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 1, 2024

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

@github-actions github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driskull driskull driskull approved these changes

@jcfranco jcfranco Awaiting requested review from jcfranco

Assignees

@Elijbet Elijbet

Labels
enhancement Issues tied to a new feature or request. Stale Issues or pull requests that have not had recent activity.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Elijbet @alisonailea @jcfranco @driskull