Skip to content

feat(website,docs): add options as a searchable dropdown to the metadata docs and use new template#6640

Open
anna-parker wants to merge 15 commits into
mainfrom
add_options_to_docs
Open

feat(website,docs): add options as a searchable dropdown to the metadata docs and use new template#6640
anna-parker wants to merge 15 commits into
mainfrom
add_options_to_docs

Conversation

@anna-parker

@anna-parker anna-parker commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

resolves #6639

Screenshot

Screen.Recording.2026-06-16.at.16.42.55.mov

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: https://add-options-to-docs.loculus.org

@anna-parker anna-parker added the preview Triggers a deployment to argocd label Jun 9, 2026
@anna-parker anna-parker marked this pull request as ready for review June 9, 2026 16:57
@anna-parker anna-parker marked this pull request as draft June 9, 2026 16:57
@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @anna-parker's task in 1m 16s —— View job


Code Review

  • Gather context and read changed files
  • Post inline comments
  • Post overall review

The implementation is clean and follows existing patterns. Two minor issues flagged as inline comments:

  1. Performance (line 322): filtered recomputes on every render — should be useMemo.
  2. Accessibility (line 327): <input> has no aria-label, only a placeholder.

Testing: No automated tests were added for the new AllowedValuesList component (the PR checklist confirms this). Given this is a docs UI component, manual verification via the preview URL is reasonable, but a snapshot or interaction test would be good to have.

Comment thread website/src/components/OrganismMetadataTable/OrganismMetadataTable.tsx Outdated
Comment thread website/src/components/OrganismMetadataTable/OrganismMetadataTable.tsx Outdated
@anna-parker anna-parker marked this pull request as ready for review June 9, 2026 17:06
@claude

claude Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @anna-parker's task in 1m 10s —— View job


Code Review

  • Gather context and read changed files
  • Analyze code for quality, security, performance, testing, docs
  • Post inline comments
  • Post overall review

The implementation is clean and well-scoped. Two issues flagged as inline comments:

  1. Performance (line 321): filtered recomputes on every render — wrap in useMemo.
  2. Accessibility (lines 325–328): <span> is not programmatically associated with the <input>. Replace with <label htmlFor> — this also subsumes the aria-label fix suggested in the previous review.

Testing: No automated tests were added (noted in the PR checklist). For a docs UI component, manual verification via the preview URL is reasonable. If a test is added later, an interaction test checking that typing in the search box filters the list would be the most valuable scenario to cover.

No security concerns — option names are rendered as text content, so no XSS risk.

Comment thread website/src/components/OrganismMetadataTable/OrganismMetadataTable.tsx Outdated
Comment thread website/src/components/OrganismMetadataTable/OrganismMetadataTable.tsx Outdated
Comment thread website/src/components/OrganismMetadataTable/OrganismMetadataTable.tsx Outdated
@theosanderson theosanderson removed the preview Triggers a deployment to argocd label Jun 15, 2026
anna-parker and others added 8 commits June 16, 2026 16:03
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
…able.tsx

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@anna-parker anna-parker force-pushed the add_options_to_docs branch from c4be671 to b9bedc1 Compare June 16, 2026 14:03
@anna-parker anna-parker added the preview Triggers a deployment to argocd label Jun 16, 2026
@anna-parker anna-parker changed the title feat(website,docs): add options as a searchable dropdown to the metad… feat(website,docs): add options as a searchable dropdown to the metadata docs and use new template Jun 16, 2026
@anna-parker anna-parker requested a review from tombch June 16, 2026 14:45
@anna-parker

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @anna-parker's task in 1m 49s —— View job


Code Review

  • Gather context and read changed files
  • Analyze code for quality, correctness, and accessibility
  • Post review findings

Good iteration — the three issues from the previous rounds (useMemo, <label htmlFor>, whitespace trimming) are all addressed. The new test file is solid. One real bug remains, plus two minor items.

Bug: duplicate id when multiple fields have options

AllowedValuesList hardcodes id='allowed-values-search' on the <input> (line 349) and htmlFor='allowed-values-search' on the <label> (line 344). When a single page renders several enum fields simultaneously, every instance shares the same id, which is invalid HTML and breaks the label→input association for all but the first element.

React 18's useId (already used in TextField.tsx, MultiFieldSearchField.tsx, etc.) is the right fix:

export const AllowedValuesList: FC<AllowedValuesListProps> = ({ options }) => {
    const inputId = useId();
    // ...
    return (
        <div className='flex flex-col gap-1'>
            <label htmlFor={inputId} ...>Search available options</label>
            <input id={inputId} ... />

Fix this →

Minor: dead aria-label prop on AllowedValuesList (line 417)

<AllowedValuesList
    options={field.options}
    values for ${field.name}`}  // ← not in AllowedValuesListProps
/>

AllowedValuesListProps only declares options, so this prop is silently discarded — TypeScript accepts aria-* on any JSX element, but AllowedValuesList never reads it. Either remove it, or add it to the type and apply it to the <ul> as aria-label (which would also give screen readers a useful label for the results list).

Minor: unguarded setTimeout in handleCopy (lines 338–340)

const handleCopy = async () => {
    await navigator.clipboard.writeText(filtered.map((o) => o.name).join('\n'));
    setCopied(true);
    setTimeout(() => setCopied(false), 2000);  // ← no cleanup
};

If the component unmounts within 2 s of clicking Copy, this fires setCopied on an unmounted component. A useRef to track the timeout and a cleanup useEffect prevents the warning:

const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
// ...
const handleCopy = async () => {
    await navigator.clipboard.writeText(filtered.map((o) => o.name).join('\n'));
    setCopied(true);
    if (timerRef.current) clearTimeout(timerRef.current);
    timerRef.current = setTimeout(() => setCopied(false), 2000);
};
useEffect(() => () => { if (timerRef.current) clearTimeout(timerRef.current); }, []);

What's now fixed ✓

  • useMemo wraps filtered
  • <label htmlFor> / id wired up ✓
  • Whitespace trimming via trimmedQuery
  • Tests added for all new behaviours including the whitespace edge cases ✓

disabled={!isClient}
className='flex-1 border border-gray-300 rounded px-2 py-1 text-sm focus:outline-none focus:ring-1 focus:ring-primary-500 disabled:bg-gray-100'
/>
<Button

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I looked at the preview I really didn't know what this Copy button would do. I would have expected it to maybe copy what I put in the search box. I would maybe put it as a button styled as a small link under the box, then it also goes with the 2 of xx so it makes sense that it will only copy what's shown

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will play around with this!

@theosanderson theosanderson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Generally looks good!

@theosanderson

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0840f256b9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

</label>
<div className='flex gap-2'>
<input
id='allowed-values-search'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Generate a unique id for each options search input

This hard-coded id is rendered by every AllowedValuesList, so metadata pages with more than one controlled-vocabulary field produce duplicate DOM ids. In that scenario every Search available options label points at the first input instead of the list it belongs to, which breaks label focus behavior and makes the controls ambiguous for assistive tech; use a per-instance id (for example via useId or a field-specific prop).

Useful? React with 👍 / 👎.

Comment thread website/src/components/OrganismMetadataTable/OrganismMetadataTable.tsx Outdated
Comment thread website/src/components/OrganismMetadataTable/OrganismMetadataTable.tsx Outdated
…able.tsx

Co-authored-by: Thomas Brier <46268349+tombch@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preview Triggers a deployment to argocd

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Docs: add dropdown, searchable list of options in docs for metadata fields with a list of allowed options

3 participants