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

feat(tw): init listbox component #4421

Merged
merged 52 commits into from
Jan 28, 2025
Merged

feat(tw): init listbox component #4421

merged 52 commits into from
Jan 28, 2025

Conversation

davidruvolo51
Copy link
Contributor

@davidruvolo51 davidruvolo51 commented Oct 31, 2024

What are the main changes you did:

how to test:

todo:

  • updated docs in case of new feature
  • added/updated tests
  • added/updated testplan to include a test for this fix, including ref to bug using # notation
  • Add support for Objects, booleans, and numbers as input.

@davidruvolo51
Copy link
Contributor Author

See issue #4446

@davidruvolo51 davidruvolo51 marked this pull request as ready for review November 12, 2024 14:50
@davidruvolo51
Copy link
Contributor Author

Need to add docs

Copy link
Member

@mswertz mswertz left a comment

Choose a reason for hiding this comment

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

I would want to make sure that CheckboxGroup, RadioGroup, Lisstbox share types where they can.

And then the make the 'ref' version of those a wrapper around them as you can see in the RefInput PR #4585

apps/metadata-utils/src/types.ts Show resolved Hide resolved
const refData = ref();

onMounted(async () => {
if (["ONTOLOGY", "ONTOLOGY_ARRAY"].includes(props.column.columnType)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't want this here.

Copy link
Member

Choose a reason for hiding this comment

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

meanwhile I don't mind this here temporarily if we have that mutual understanding.

@mswertz mswertz dismissed their stale review January 23, 2025 08:50

so many chages, need new approval

@mswertz
Copy link
Member

mswertz commented Jan 27, 2025

great, in discussion we concluded:

  • listbox doesn't need multiple select now, we will address this outside (e.g. using filter wells that user can the remove; every select results in more wells)
  • search we will do later. We envision a slot for the search for use cases where search need to be in the select. The handling of search events can then handled outside to filter of options.
  • we will remove the form integration for now and move that to the refinput PR

Copy link
Member

@mswertz mswertz left a comment

Choose a reason for hiding this comment

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

my main requested changes is to approach listbox as part of a family of option select inputs, together with checkboxgroup and radiogroup. So make sure their interface is the same.

In addition I am confused by your use of refs to link state of the list items. I would have expected simply to use emit(update:modelValue) to happen in the list. I think this non-standard approach makes maintenance more complex and has the risk of unforseen side effects as we might bypass the normal vue state/refresh handling.

@@ -104,3 +104,10 @@ export type columnValue = string | number | boolean | columnValueObject;
interface columnValueObject {
[x: string]: columnValue;
}

export type IInputValue = string | number | boolean;
Copy link
Member

Choose a reason for hiding this comment

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

I think the IInputValue should be integrated with columnValue types?

  • primitiveColumValue = string | number | boolean;
  • columnValue = primitiveColumnValue | columnValueObject | columnValueObject[]
  • columnValueObject = [x: string]: columnValue

apps/tailwind-components/assets/css/main.css Show resolved Hide resolved
const refData = ref();

onMounted(async () => {
if (["ONTOLOGY", "ONTOLOGY_ARRAY"].includes(props.column.columnType)) {
Copy link
Member

Choose a reason for hiding this comment

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

meanwhile I don't mind this here temporarily if we have that mutual understanding.


export type IInputValue = string | number | boolean;

export type IInputValueLabel = {
Copy link
Member

Choose a reason for hiding this comment

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

can we please merge this type with the same time for checkboxgroup and radiogroup?

}

export interface IListboxLiRef {
li: HTMLLIElement;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we would need this html element management?

Copy link
Member

@mswertz mswertz left a comment

Choose a reason for hiding this comment

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

we discussed that ref are needed for aria behavior and that we will do other refactorings after the merge.

@davidruvolo51 davidruvolo51 requested review from chinook25, mswertz and connoratrug and removed request for chinook25, connoratrug and mswertz January 28, 2025 10:17
@davidruvolo51 davidruvolo51 dismissed stale reviews from connoratrug and chinook25 January 28, 2025 10:29

Merging now and will refactor in the next PR

@davidruvolo51 davidruvolo51 merged commit c439a85 into master Jan 28, 2025
6 of 7 checks passed
@davidruvolo51 davidruvolo51 deleted the feat/tw-listbox branch January 28, 2025 10:30
connoratrug added a commit that referenced this pull request Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants