-
-
Notifications
You must be signed in to change notification settings - Fork 5
Add part of speech filter #2079
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
Conversation
📝 WalkthroughWalkthroughThis PR enhances field editors (select and multi-select) with improved handling of missing labels using "Untitled" fallbacks, ellipsis overflow styling, and adjusted command list heights. A new PartOfSpeechSelect component is introduced for grammatical filtering in the browse interface, integrated into SearchFilter. Translations are added across multiple locales for the new UI strings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
myieye
left a comment
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.
The "Part of speech" field is called "Grammatical info." in the "FieldWorks (Classic)" view.
You used the correct terminology for the FieldWorks Lite view, but need to adapt the FieldWorks (Classic) view.
Please find a good way to use that terminology where you're currently referring to "Parts of speech".
|
@copilot see my review |
Updated to use view-based terminology in commit 8dccced:
This matches the terminology used in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: myieye <12587509+myieye@users.noreply.github.com>
Co-authored-by: myieye <12587509+myieye@users.noreply.github.com>
Use "Grammatical info." for FieldWorks Classic view and "Part of speech" for FieldWorks Lite view in both the filter label and placeholder text. Co-authored-by: myieye <12587509+myieye@users.noreply.github.com>
41084f2 to
5a86f84
Compare
myieye
left a comment
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.
Ran into a little bit of scope creep as I found various nuisances, imperfections in our select components.
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
frontend/viewer/src/lib/components/field-editors/multi-select.svelte(4 hunks)frontend/viewer/src/lib/components/field-editors/select.svelte(3 hunks)frontend/viewer/src/lib/components/ui/button/x-button.svelte(1 hunks)frontend/viewer/src/locales/en.po(4 hunks)frontend/viewer/src/locales/es.po(4 hunks)frontend/viewer/src/locales/fr.po(4 hunks)frontend/viewer/src/locales/id.po(4 hunks)frontend/viewer/src/locales/ko.po(4 hunks)frontend/viewer/src/locales/ms.po(4 hunks)frontend/viewer/src/locales/sw.po(4 hunks)frontend/viewer/src/project/browse/SearchFilter.svelte(5 hunks)frontend/viewer/src/project/browse/filter/PartOfSpeechSelect.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
📚 Learning: 2025-06-18T05:13:00.591Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Applied to files:
frontend/viewer/src/lib/components/field-editors/multi-select.sveltefrontend/viewer/src/project/browse/SearchFilter.sveltefrontend/viewer/src/project/browse/filter/PartOfSpeechSelect.sveltefrontend/viewer/src/lib/components/field-editors/select.svelte
📚 Learning: 2025-07-30T04:53:41.702Z
Learnt from: rmunn
Repo: sillsdev/languageforge-lexbox PR: 1844
File: frontend/viewer/src/lib/entry-editor/ItemListItem.svelte:26-37
Timestamp: 2025-07-30T04:53:41.702Z
Learning: In frontend/viewer/src/lib/entry-editor/ItemListItem.svelte, the TODO comments for unused props `index` and `actions` are intentional reminders for future work to be completed in a separate PR, not issues to be resolved immediately. These represent planned functionality that will be implemented later.
Applied to files:
frontend/viewer/src/lib/components/field-editors/multi-select.sveltefrontend/viewer/src/locales/en.pofrontend/viewer/src/lib/components/field-editors/select.sveltefrontend/viewer/src/locales/sw.po
📚 Learning: 2025-06-02T14:27:02.745Z
Learnt from: myieye
Repo: sillsdev/languageforge-lexbox PR: 1720
File: frontend/viewer/src/locales/es.json:1786-1790
Timestamp: 2025-06-02T14:27:02.745Z
Learning: Spanish locale file (frontend/viewer/src/locales/es.json) contains generated code that may have null line numbers in origin entries due to limitations in the code generation process.
Applied to files:
frontend/viewer/src/locales/es.po
📚 Learning: 2025-10-06T12:48:36.601Z
Learnt from: myieye
Repo: sillsdev/languageforge-lexbox PR: 2025
File: frontend/viewer/src/locales/sw.po:146-149
Timestamp: 2025-10-06T12:48:36.601Z
Learning: Do not review or comment on `.po` translation files (e.g., files in `frontend/viewer/src/locales/`) in the languageforge-lexbox repository, as translations are managed by Crowdin.
Applied to files:
frontend/viewer/src/locales/es.pofrontend/viewer/src/locales/en.po
📚 Learning: 2025-08-15T07:32:23.641Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1923
File: frontend/viewer/src/locales/en.po:845-848
Timestamp: 2025-08-15T07:32:23.641Z
Learning: Do not suggest specific translations for missing msgstr entries in locale files. Only identify that translations are missing without proposing the actual translated text.
Applied to files:
frontend/viewer/src/locales/es.po
🔇 Additional comments (14)
frontend/viewer/src/lib/components/ui/button/x-button.svelte (1)
15-15: LGTM! Background styling improvement.The addition of
bg-backgroundensures consistent background styling for the close button across different contexts.frontend/viewer/src/lib/components/field-editors/multi-select.svelte (4)
166-183: LGTM! Good UX improvements for missing labels.The addition of
overflow-hidden, ellipsis styling, and the "Untitled" fallback text for missing labels improves the component's robustness and visual presentation.
216-216: LGTM! Improved list height constraints.The responsive height constraints (
max-md:h-[300px] md:max-h-[40vh]) ensure the command list remains usable on different screen sizes without occupying too much vertical space.
227-248: LGTM! Consistent fallback styling.The conditional class for missing labels and the "Untitled" fallback text provide consistent visual feedback when option labels are empty.
225-225: No breaking changes detected—original concern is not valid.The
getHighlightedValue()function retrieves values via thedata-value-indexattribute (line 160) and returns the corresponding entry fromfilteredOptions(line 161), not the CommandItem value prop itself. This means the change fromlabeltolabel.toLocaleLowerCase() + String(id)only affects bits-ui's internal command filtering behavior and does not impact how the multi-select component or external code accesses selected values. No external dependencies on the CommandItem value format were found.frontend/viewer/src/project/browse/SearchFilter.svelte (5)
22-24: LGTM! Proper imports for the new filter.The imports for
PartOfSpeechSelectandIPartOfSpeechtype are correctly added.
46-46: LGTM! State variable follows existing patterns.The
partOfSpeechstate variable declaration is consistent with thesemanticDomainstate variable above it.
79-81: LGTM! Filter logic is correct.The part of speech filter construction (
Senses.PartOfSpeechId=${partOfSpeech.id}) correctly filters entries by the selected part of speech ID, following the same pattern as the semantic domain filter.
124-124: LGTM! Improved scrollability for filter section.Increasing the max-height to
max-h-[calc(65vh-3rem)]with overflow-y-auto ensures the expanded filter section remains scrollable and doesn't dominate the viewport, especially important now with the additional part of speech filter.
150-153: LGTM! Well-integrated part of speech filter UI.The part of speech filter section follows the same pattern as the semantic domain filter above it, with appropriate labeling using view-specific terminology ("Grammatical info." for classic view, "Part of speech" for lite view).
frontend/viewer/src/project/browse/filter/PartOfSpeechSelect.svelte (1)
1-23: LGTM! Clean implementation of the part of speech filter.The component follows established patterns and integrates well with existing services (writing system, parts of speech, current view). The use of
pickBestAlternativefor label selection andpt()for localized placeholders is appropriate.frontend/viewer/src/lib/components/field-editors/select.svelte (3)
97-106: Good UX improvements for label handling.The addition of ellipsis truncation and the "Untitled" fallback ensures consistent display when labels are missing or too long. The
mr-4margin provides proper spacing for the chevron/clear buttons.
132-132: Height constraint adjustment improves UX.Reducing the max height from 50vh to 40vh prevents the dropdown from dominating the screen, which aligns with the PR's goal of making filters more manageable.
136-149: The value composition change is safe and consistent across the codebase.The value prop in CommandItem is used internally by the Command component for filtering and selection, not parsed by external code. Both
select.svelteandmulti-select.svelteconsistently use the same pattern (label.toLocaleLowerCase() + String(id)), and theonSelecthandlers work with the original option object via callback, not the value string. No breaking changes detected.
imnasnainaec
left a comment
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.
😃 The code looks good.
❓ Could we get some screenshots to show key visual changes?
@imnasnainaec there are actually screenshots already at the top. |
@myieye Indeed there are! I saw them at one point, but I think I mentally dismissed them, assuming they hadn't been updated for any of the various subsequent ui tweaks. |
Adds a part of speech filter to the browse view, matching the visual design of the existing semantic domain filter.
The collapsible filter section also now has a max-height and scrolls, so it doesn't hog the whole screen:
