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

Add new Menu Component #2933

Merged
merged 138 commits into from
Mar 27, 2023
Merged

Add new Menu Component #2933

merged 138 commits into from
Mar 27, 2023

Conversation

NillerW
Copy link
Collaborator

@NillerW NillerW commented Mar 8, 2023

Which issue does this PR close?

This PR closes #2744 and #2807

What is the new behavior?

This PR introduces a the Menu component. Menu component consists of a button, which by default can be clicked to show a floating list of Kirby-items.

Does this PR introduce a breaking change?

  • Yes
  • No

Are there any additional context?

A showcase has been added to display the new menu:

image
image
image

Checklist:

The following tasks should be carried out in sequence in order to follow the process of contributing correctly.

Reminders

  • Make sure you have implemented tests following the guidelines in: "The good: Test".
  • Make sure you have updated the cookbook with examples and showcases (for bug fixes, enhancements & new components).

Review

  • Determine if your changes are a fix, feature or breaking-change, and add the matching label to your PR. If it is tooling, dependency updates or similar, add ignore-for-release.
  • Do a self-review.
  • Request that the changes are code-reviewed
  • Request that the changes are UX reviewed (only necessary if your PR introduces visual changes)

When the pull request has been approved it will be merged to develop by Team Kirby.

@kodeaben kodeaben force-pushed the component/2744-action-list branch from 1df8948 to 925937a Compare March 24, 2023 09:49
@github-actions github-actions bot temporarily deployed to pr-2744-action-list March 24, 2023 11:36 Inactive
@github-actions github-actions bot temporarily deployed to pr-2744-action-list March 24, 2023 11:54 Inactive
@github-actions github-actions bot temporarily deployed to pr-2744-action-list March 24, 2023 12:46 Inactive
This could for example be one of the top elements in the DOM. This is not generalized, since
each project may have different tags for which they want the content to be placed under. This
approach is also to prevent the need for adjusting the DOM with an extra base element, when the
need for portal arises.
Copy link
Contributor

Choose a reason for hiding this comment

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

Her er et bud på en anden formulering:

In some cases, like when using the menu inside an item, you might experience that the menu doesn't show when clicked. This is due to the menu being placed in a "lower" part of the stacking context.

To solve this, we need to move the menu further up, and we do this by using a "portal".
When using a portal, we tell the DOM to attach out menu to a specific element. Which element to attatch the menu to depends on the architecture of your application, but a rule of thumb is to pick an element which only exists once in the DOM

expected from an item in the list, is up to the user to implement. See examples.
</p>
<p>
By default a simple button with the icon 'more' is used. A custom kirby-button can be provided
Copy link
Contributor

Choose a reason for hiding this comment

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

let's wrap tags in < code ></ code > tags

which takes in a
<code>HTMLElement</code>
. It is your task to provide a reference to the DOM element for which you want to portal the
menu content into. This can example be achieved by using:
Copy link
Contributor

Choose a reason for hiding this comment

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

"This can for example be achieved by using"

import { FloatingDirective } from '@kirbydesign/designsystem/shared/floating';
import { MenuComponent } from './menu.component';

describe('ActionListComponent', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename ;)

});
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like the tests you have written! They are easy to understand covers the basic!

I'm however missing a few more tests which would cover the usecases such as the user providing its own button.
We can talk this through and find out what we might also like to test.

<h3>Menu placement</h3>
<p>
By default the menu will open, having the content shown to the right. This might not be optimal
for a menu placed far right on the screen. To ensure enough place on the screen for the content
Copy link
Contributor

Choose a reason for hiding this comment

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

did you means space instead of place?

<h3>Advanced items</h3>
<p>
Kirby-items can be more that just a clickable item. By modifying the menu to not close on select,
we can interact with the items toggle without the menu closing
Copy link
Contributor

Choose a reason for hiding this comment

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

missing a "." at the end

@kodeaben kodeaben merged commit fb98764 into develop Mar 27, 2023
@kodeaben kodeaben deleted the component/2744-action-list branch March 27, 2023 08:52
@RasmusKjeldgaard RasmusKjeldgaard changed the title Component/2744 Menu Add new Menu Component Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component feature Add this PR to the changelog as a feature
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[COMPONENT] Menu
5 participants