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

refactor(components): menu item migrate to fast #636

Merged
merged 52 commits into from
Sep 22, 2022
Merged

Conversation

yinonov
Copy link
Contributor

@yinonov yinonov commented Aug 29, 2022

this PR introduces a new method circumventing the context lock-down to plain text by assigning textContent member to template instead of the convention of text property

@yinonov yinonov changed the title refactor(components): menu migrate to fast refactor(components): menu item migrate to fast Aug 30, 2022
@yinonov yinonov self-assigned this Sep 4, 2022
@yinonov yinonov marked this pull request as draft September 9, 2022 07:15

```html preview center
`placement` property from _popup_ propagate through _menu_ and sets its position in accordance to its anchor.
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that the position is according to the vwc-button, but it doesn't have the height of the button itself so the placement is strange:
this is right-start:
image

this is the same but with adding display:inline-block:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue in popup?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the popup is OK, it sets the position relative to the anchor and the vwc-button as an anchor had a strange height cause its not "block" element.

Copy link
Contributor Author

@yinonov yinonov Sep 20, 2022

Choose a reason for hiding this comment

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

oh, should be fixed in button then. opened #695

libs/components/src/lib/menu-item/README.md Outdated Show resolved Hide resolved
box-sizing: border-box;
align-items: center;
background-color: var(#{appearance.get-appearance-token(fill)});
block-size: $min-block-size;
Copy link
Contributor

Choose a reason for hiding this comment

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

if there's break-word than we cant have a block-size. maybe min-block-size?
image

in vivid-2 we do not break lines, but have ellipses

font: var(#{constants.$vvd-font-base});
gap: $gap;
hyphens: auto;
inline-size: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is great - but doesn't get the width that is set on menu....
The menu width + line break is not working well :(

`anchor` property from _popup_ propagate through _menu_ and sets its anchor reference.

- Type: `string`
- Default: `''`

```html preview center
<div style="position: relative">
Copy link
Contributor

Choose a reason for hiding this comment

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

the preview is cut off - maybe add style for the html to set a preloaded height:

image

rachelbt
rachelbt previously approved these changes Sep 21, 2022
Copy link
Contributor

@rachelbt rachelbt left a comment

Choose a reason for hiding this comment

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

saw the related issues that you open regarding issues related.
LGTM at this point :)

@yinonov yinonov dismissed rinaok’s stale review September 22, 2022 14:20

comments resolved

@yinonov yinonov merged commit 84288bd into main Sep 22, 2022
@yinonov yinonov deleted the menu-migration-to-fast branch September 22, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[menu-item] migrate to vivid-next
3 participants