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

[Feature] Select component for Codex UI #279

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

DeadCreator
Copy link

Added Dropdown component implementation (#273), but in component only one option is selectable. Also updated icons package version in package json's

Added Dropdown component implementation, but in component only one option is selectable. Also updated icons package version in package json's
@DeadCreator DeadCreator self-assigned this Nov 28, 2024
@DeadCreator DeadCreator requested a review from neSpecc November 28, 2024 20:58
>
{{ activeItem.title }}
</Button>
<div :style="showPopover ? {'display' : 'block'} : {'display': 'none'}">
Copy link
Member

Choose a reason for hiding this comment

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

use the Popover component as a wrapper

@e11sy
Copy link
Contributor

e11sy commented Nov 28, 2024

why file in dev/pages is called Select.vue if component name is dropdown...

Changed Component's name from Select to Dropdown;
Converting options list into Popover component
@DeadCreator DeadCreator requested review from neSpecc and e11sy December 2, 2024 15:19
e11sy
e11sy previously requested changes Dec 3, 2024
Comment on lines 79 to 84
&__btn {
user-select: none;
background-color: var(--base--bg-secondary);
margin-bottom: var(--spacing-s);
padding-right: var(--spacing-s);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

seems redundant

Comment on lines 85 to 89
&__btn:hover {
background-color: var(--base--bg-secondary-hover);
cursor: pointer;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Comment on lines 89 to 90
path: '/components/dropdown',
component: Dropdown as Component,
Copy link
Contributor

Choose a reason for hiding this comment

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

why select component is removed? you need to add new dropdown component, not to override select

if (item.type === 'default' || !item.type) {
activeItem.value = Object.create(item);
// eslint-disable-next-line no-console
activeItem.value.onActivate = console.log;
Copy link
Contributor

Choose a reason for hiding this comment

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

onActivate will recieve no parameters, so logs would be empty.

Anyway, i think, that this loggs are unneeded in app, where this component would be used

Copy link
Contributor

@e11sy e11sy Dec 3, 2024

Choose a reason for hiding this comment

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

also this works wrong:

  • If i open popover by pressing dropdown button, then isOpened becomes true
  • Then i select any option in context menu, popover becomes hidden, and title of the button updates
  • If i press popover button one more time it makes isOpened false and nothing changes on page (i expect popover to be opened again)

so we should manually hide() popover, when new item selected or popover closed by pressing anywhere else

@DeadCreator DeadCreator dismissed e11sy’s stale review December 3, 2024 19:02

Didn't make all changes

There was some mess with component's name, because Dropdown and Select lots in common, so now this is Select component

Also added ability to toggle select by clicking on selection button
@DeadCreator DeadCreator changed the title [Feature] Dropdown component for Codex UI [Feature] Select component for Codex UI Dec 9, 2024
@DeadCreator DeadCreator requested review from e11sy and removed request for neSpecc and e11sy December 9, 2024 20:27
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