-
Notifications
You must be signed in to change notification settings - Fork 47
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
pkp/pkp-lib#10033 Add new dropdown menu component #363
Conversation
|
||
const computedAriaLabel = computed(() => { | ||
return props.ariaLabel || props.name || 'More actions'; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'More actions' needs to be localised string. But maybe we could make name required? So it would be required also when the isEllipsis option is used? Maybe would call it as displayAsEllipsis to be bit more descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, we will require the "name" prop and rename it to "label" to make it consistent to other components. I updated this now.
> | ||
<MenuItems | ||
class="dropdown-shadow absolute z-10 w-max border border-light bg-secondary focus:outline-none" | ||
:class=" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using 'shadow' classes that we have configured in tailwind? Its slightly smaller shadow - but if we would want to introduce additional shadow we would add it as variant to tailwind..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted introducing a new class and instead used the existing "shadow" class.
control: {type: 'select'}, | ||
options: ['left', 'right'], | ||
description: | ||
'Determines where to show the dropdown button. Options include `left` and `right`.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aiming to have the documentation collocated with the component. Storybook si pretty good in picking up jsdoc.. So at least description and options should go there.. Not sure if will be possible to make it 'type Select' just from jsdoc, but please check if thats possible.
You can see for example in Button.vue the comments which are than displayed in storybook automatically. There has been some improvements in latest storybook - so not sure whether there is more than description that could be conveyed..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jardakotesovec , I checked this and atm, the controls to use "Select" and options cannot be done via jsdoc.. So I have to keep it on the Storybook file for the component. However, the documentation for this is on the jsdoc, so we were able to just define this on the Story file:
argTypes: {
direction: {
control: {type: 'select'},
options: ['left', 'right'],
},
},
:href="action.url" | ||
:icon="action.icon" | ||
:is-active="active" | ||
:is-warnable="action.isWarnable" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actions rarely will be links - mostly it will be actions that just needs to be emitted and handled by the parent component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now addressed, please see latest updates on the PR :)
…pdownActions component
No description provided.