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(components): Filter suggestion dropdown to only include values that exist with the current filters and add show the number of options #633

Closed
wants to merge 3 commits into from

Conversation

anna-parker
Copy link
Collaborator

@anna-parker anna-parker commented Jan 2, 2025

resolves #609

Summary

This shows the number of values for each option in the dropdown and filters the list of option to options that are available with current filters.

Sadly it is not possible to style the options component so that the number of the options is next to the value and not below, we will need to create a custom component for this. Loculus does this by using '@headlessui/react''s Combobox class -- however sadly this requires react and thus cannot be used in our webcomponents (?) I have not been able to find a good solution for this without react.

Screenshot

image

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by an appropriate test.

Copy link

vercel bot commented Jan 2, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
dashboard-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 3, 2025 6:57pm

Copy link
Contributor

github-actions bot commented Jan 2, 2025

This is a preview of the changelog of the next release. If this branch is not up-to-date with the current main branch, the changelog may not be accurate. Rebase your branch on the main branch to get the most accurate changelog.

Note that this might contain changes that are on main, but not yet released.

Changelog:

0.11.4 (2025-01-06)

Features

  • components: add number of each option to filter options (78af02a)
  • components: add option to delete input of text-input and lineage-filter input with x and sort text-input datalist options (b58faae)
  • components: add option to filter option selection shown in text-input and lineage-filter using lapisFilter objects (e06c074)

Bug Fixes

  • components: gs-aggregate: rerender when initialSortField or initialSortDirection changes (60d578f)
  • components: prevent playwright race conditions by changing name of info header (2dabbe9)

@anna-parker anna-parker changed the title Add-number-options feat(components): Filter suggestion dropdown to only include values that exist with the current filters and add show the number of options Jan 3, 2025
…-input and lineage-filter using lapisFilter objects
<option
value={item[lapisField]}
key={item[lapisField]}
style='white-space: nowrap;'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
style='white-space: nowrap;'
className='whitespace-nowrap'

Comment on lines +124 to +127
<option
value={item[lapisField]}
key={item[lapisField]}
>{`${item[lapisField]} (${item['count']})`}</option>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we also want to include

                        className='whitespace-nowrap'

here?

value={item[lapisField]}
key={item[lapisField]}
style='white-space: nowrap;'
>{`${item[lapisField]} (${item['count']})`}</option>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need a template string here. You can use JSX string rendering directly.

Suggested change
>{`${item[lapisField]} (${item['count']})`}</option>
>{item[lapisField]} ({item['count']})</option>

<option
value={item[lapisField]}
key={item[lapisField]}
>{`${item[lapisField]} (${item['count']})`}</option>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need a template string here. You can use JSX string rendering directly.

Suggested change
>{`${item[lapisField]} (${item['count']})`}</option>
>{item[lapisField]} ({item['count']})</option>

@@ -78,7 +88,7 @@ const TextInputInner: FunctionComponent<TextInputInnerProps> = ({ lapisField, pl
if (value === undefined) {
return true;
}
return data.includes(value);
return data.map((item) => item[lapisField]).includes(value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would probably be better to put data.map((item) => item[lapisField]) in a useMemo so that we don't have to map the whole data on every keystroke again.

Comment on lines +24 to +32
if (aValue === undefined && bValue !== undefined) {
return 1;
}
if (bValue === undefined && aValue !== undefined) {
return -1;
}
if (aValue === undefined && bValue === undefined) {
return 0;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is shorter, less complicated, but most probably as good as the longer version ;)

Suggested change
if (aValue === undefined && bValue !== undefined) {
return 1;
}
if (bValue === undefined && aValue !== undefined) {
return -1;
}
if (aValue === undefined && bValue === undefined) {
return 0;
}
if (aValue === undefined) {
return 1;
}
if (bValue === undefined) {
return -1;
}

field: string,
signal?: AbortSignal,
) {
const fetchAggregatedOperator = new FetchAggregatedOperator<Record<string, string>>(lapisFilter, [field]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually the type Record<string, string> is not really correct. When you choose a field of type int for field, then the sort function won't work.

How do we deal with this? Should we generalize the sort function?

@@ -14,6 +14,7 @@ import { withinShadowRoot } from '../withinShadowRoot.story';
const codeExample = String.raw`
<gs-text-input
lapisField="host"
lapisFilter={{}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to provide an example with more values here.

@@ -29,6 +29,20 @@ export class TextInputComponent extends PreactLitAdapter {
@property()
initialValue: string | undefined = undefined;

/**
* A LAPIS filter to fetch the number of sequences for.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* A LAPIS filter to fetch the number of sequences for.
* A LAPIS filter to fetch the number of sequences that is shown on each autocomplete suggestion.

Comment on lines +16 to +43
const output: (Record<string, string> & {
count: number;
})[] = [];

const findOrCreateGroup = (pattern: string) => {
let group = output.find((item) => item[field] === pattern) as GroupType | undefined;
if (!group) {
group = { count: 0, [field]: pattern } as GroupType;
output.push(group);
}
return group;
};

data.forEach((item) => {
if (!item[field]) {
return;
}
const parts = item[field].split('.');
for (let step = 0; step < parts.length; step++) {
const section = `${parts.slice(0, step + 1).join('.')}*`;
const group = findOrCreateGroup(section);
group.count += item.count;
}
const group = findOrCreateGroup(item[field]);
group.count += item.count;
});

return sortDataByField(output, field);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this tries to do more than it can achieve:

  • The sorting is off:
    image
    JN.1.1 appears at the top, but JN.1.1* is somewhere further down. I would expect them to be next to each other.
  • The computed counts for values that include sublineages are not correct, because they don't consider aliases.
  • SILO will already return all possible parent lineage. IMO we should not need to compute them here.

This looks more complicated than initially thought. Let's split the changes?

  • Let's discuss counts for lineages and maybe do it later.
  • We can get sorting (for gs-text-input and gs-lineage-filter) and counts for gs-text-input done in this PR.

@fengelniederhammer
Copy link
Collaborator

Closing in favor of #666 🤘

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.

Filter autocomplete options of location filter, text input and location filter
2 participants