Improve tag picker UX with inline search and controller optimizations#1366
Improve tag picker UX with inline search and controller optimizations#1366IamTaoChen wants to merge 7 commits into
Conversation
- add inline search inside the tag candidate dropdown - keep selected tags in the Tags area and mark them disabled in Select - move filtering to tag-picker to avoid nested controller issues - auto-focus the search input when the dropdown opens - optimize refresh performance with tag-id maps instead of repeated lookups - keep search results in sync after select/deselect state changes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a DS::MultiSelectTags ViewComponent and ERB partial, a Stimulus Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as "UI (chips / dropdown)"
participant Controller as "Stimulus: tag-picker"
participant Form as "Hidden checkboxes / Form inputs"
User->>UI: Open dropdown
UI->>Controller: handleDropdownToggle()
Controller->>Controller: focus & select search input
User->>UI: Type search
UI->>Controller: filterOptions()
Controller->>UI: show/hide matching options / toggle empty message
User->>UI: Click option
UI->>Controller: select(event)
Controller->>Form: set checkbox.checked & dispatch change
Form-->>Controller: change event bubbles
Controller->>Controller: refresh() -> show chip, disable option
User->>UI: Click selected chip
UI->>Controller: deselect(event)
Controller->>Form: unset checkbox.checked & dispatch change
Controller->>Controller: refresh() -> hide chip, enable option
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afaa6ac04e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| **input_options | ||
| }, | ||
| tag.id, | ||
| nil %> |
There was a problem hiding this comment.
Using form.check_box ... tag.id, nil means unchecked boxes emit no value, so when a user clears every tag the request omits the tag_ids key entirely. That breaks flows that rely on explicit key presence to clear tags (e.g. Transactions::BulkUpdatesController#tags_provided?), so submitting after deselecting all tags preserves existing tags instead of removing them. This is a regression from the previous multi-select behavior that could send an explicit blank/empty tag_ids payload.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/DS/multi_select_tags.html.erb (1)
32-41: Consider adding accessible labels for chip buttons.The chip buttons allow deselecting tags but lack accessible context for screen reader users. Consider adding
aria-labelto clarify the action.Suggested improvement for accessibility
<button type="button" class="border text-sm font-medium px-3 py-1 rounded-full items-center gap-2 transition-opacity duration-150 hover:opacity-70 <%= selected?(tag) ? "inline-flex" : "hidden" %> <%= "opacity-50 pointer-events-none" if disabled %>" data-tag-picker-target="chip" data-tag-id="<%= tag.id %>" data-action="click->tag-picker#deselect" + aria-label="<%= t("helpers.select.remove_tag", tag: tag.name, default: "Remove #{tag.name}") %>" <%= "disabled" if disabled %> style="<%= selected_chip_style(color) %>">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/DS/multi_select_tags.html.erb` around lines 32 - 41, Chip buttons lack accessible labels for screen readers; update the button element rendered in multi_select_tags.html.erb to include an informative aria-label (e.g., "Deselect {tag.name}" or "Select {tag.name}" based on selected?(tag)) so screen readers know the action, and ensure the label uses the tag.name and/or tag.id for uniqueness. Modify the same button that uses data-tag-picker-target="chip", data-action="click->tag-picker#deselect", and selected_chip_style(color) to add the aria-label attribute (taking into account the disabled state if needed). Ensure you do not change the visual text (tag.name) and keep existing attributes (data-tag-id, data-action) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/transactions/bulk_updates/new.html.erb`:
- Around line 15-21: The MultiSelectTags checkbox inputs do not submit tag_ids
when all are unchecked, so update the form rendering (DS::MultiSelectTags usage)
to always send tag_ids by adding a hidden input named tag_ids with an empty
value (or a dedicated "clear" control that submits tag_ids: [""]) so the
controller's tags_provided? sees a parameter and bulk update runs; modify the
DS::MultiSelectTags component or the new.html.erb form wrapper to inject that
hidden field alongside the generated checkboxes (referencing
DS::MultiSelectTags, the tags_provided? check in the controller, and the Entry
model bulk_update! tag-update logic).
---
Nitpick comments:
In `@app/components/DS/multi_select_tags.html.erb`:
- Around line 32-41: Chip buttons lack accessible labels for screen readers;
update the button element rendered in multi_select_tags.html.erb to include an
informative aria-label (e.g., "Deselect {tag.name}" or "Select {tag.name}" based
on selected?(tag)) so screen readers know the action, and ensure the label uses
the tag.name and/or tag.id for uniqueness. Modify the same button that uses
data-tag-picker-target="chip", data-action="click->tag-picker#deselect", and
selected_chip_style(color) to add the aria-label attribute (taking into account
the disabled state if needed). Ensure you do not change the visual text
(tag.name) and keep existing attributes (data-tag-id, data-action) intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14e2b17a-b75b-4169-b821-0dafd028223f
📒 Files selected for processing (7)
app/components/DS/multi_select_tags.html.erbapp/components/DS/multi_select_tags.rbapp/javascript/controllers/tag_picker_controller.jsapp/views/transactions/_form.html.erbapp/views/transactions/bulk_updates/new.html.erbapp/views/transactions/searches/filters/_tag_filter.html.erbapp/views/transactions/show.html.erb
- Add hidden field to ensure tag_ids key is always submitted when all tags are deselected, preventing a regression where bulk update could not clear tags - Validate hex color format in tag_color() to prevent style injection - Use inline opacity style instead of toggling Tailwind hover:* classes in JS, which are not compiled by JIT in production - Replace hardcoded "Tags" label in bulk_updates/new with i18n lookup - Add helpers.select.no_results locale key and remove fallback default
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/DS/multi_select_tags.html.erb (1)
3-7: Use a<fieldset>/<legend>for this composite control.
form.labelsemantically targets a single input, but this widget is a group of chips, a disclosure, a search field, and option buttons. Wrapping the picker in a<fieldset>with a<legend>will give assistive tech a stable group label.As per coding guidelines,
**/*.html.erb: "Prefer native HTML elements over JavaScript-based components. Use semantic HTML".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/DS/multi_select_tags.html.erb` around lines 3 - 7, Replace the single input label with a semantic fieldset/legend group: wrap the composite picker markup (the container with class "form-field" / "form-field__body" and all child controls — chips, disclosure, search field, option buttons) inside a <fieldset>, and render the existing label as a <legend> (use the same class "form-field__label" and preserve the label content when the label variable is present instead of calling form.label). Ensure the rest of the markup remains inside the fieldset so assistive tech sees the group label for the multi-control widget.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/DS/multi_select_tags.html.erb`:
- Around line 15-28: The invisible checkbox inputs are still focusable via
keyboard because they use the "sr-only" class; update the form.check_box call in
multi_select_tags.html.erb (the call that uses checkbox_id(tag), selected?(tag),
and data-action "change->tag-picker#refresh") to remove the "sr-only" class and
instead add tabindex: "-1" and "aria-hidden": "true" (so they are removed from
the tab order and accessibility tree) while keeping the existing data-*
attributes and value arguments intact; this ensures the underlying inputs remain
usable for form submission/JS but are not keyboard-focusable or announced before
the visible picker buttons.
- Around line 10-11: The hidden sentinel input (the call to form.hidden_field
method, multiple: true, value: "") always submits an empty tag_ids and can
unintentionally clear tags when the picker is disabled or when callers rely on
omission semantics; update the template to render or enable that hidden_field
only when the picker is active and/or when callers explicitly request it.
Concretely: wrap the hidden_field in a conditional that checks the disabled
state (the same flag used to render the checkboxes near the method inputs) and
add an opt-out option (e.g., local include_hidden or opts[:include_hidden]
defaulting to true) so callers can pass include_hidden: false to omit the
sentinel; when the picker is disabled, do not render (or render disabled) the
hidden_field so it cannot clear tags unintentionally. Ensure the change touches
the hidden_field invocation and uses the existing method/disabled variables used
for the checkboxes so behavior is consistent.
---
Nitpick comments:
In `@app/components/DS/multi_select_tags.html.erb`:
- Around line 3-7: Replace the single input label with a semantic
fieldset/legend group: wrap the composite picker markup (the container with
class "form-field" / "form-field__body" and all child controls — chips,
disclosure, search field, option buttons) inside a <fieldset>, and render the
existing label as a <legend> (use the same class "form-field__label" and
preserve the label content when the label variable is present instead of calling
form.label). Ensure the rest of the markup remains inside the fieldset so
assistive tech sees the group label for the multi-control widget.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f76b07eb-122b-4438-9f81-05452f52a579
📒 Files selected for processing (6)
app/components/DS/multi_select_tags.html.erbapp/components/DS/multi_select_tags.rbapp/javascript/controllers/tag_picker_controller.jsapp/views/transactions/bulk_updates/new.html.erbconfig/locales/defaults/en.ymlconfig/locales/views/transactions/en.yml
✅ Files skipped from review due to trivial changes (2)
- config/locales/views/transactions/en.yml
- config/locales/defaults/en.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/transactions/bulk_updates/new.html.erb
- app/components/DS/multi_select_tags.rb
| <%# Ensure the key is always submitted even when all checkboxes are unchecked %> | ||
| <%= form.hidden_field method, multiple: true, value: "" %> |
There was a problem hiding this comment.
Don’t always submit the clearing sentinel.
Line 11 always posts tag_ids[]= even when the picker is disabled or intentionally untouched. In disabled mode, the real checkboxes on Line 21 will not submit, so this hidden input can clear existing tags by itself. It also removes the bulk-edit ability to preserve tags by omitting tag_ids. Make this hidden field conditional/disabled alongside the inputs, and keep an opt-out for flows that rely on omission semantics.
Based on learnings, omitting the tag_ids key entirely ... preserves existing tags; sending tag_ids: [""] ... should clear tags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/components/DS/multi_select_tags.html.erb` around lines 10 - 11, The
hidden sentinel input (the call to form.hidden_field method, multiple: true,
value: "") always submits an empty tag_ids and can unintentionally clear tags
when the picker is disabled or when callers rely on omission semantics; update
the template to render or enable that hidden_field only when the picker is
active and/or when callers explicitly request it. Concretely: wrap the
hidden_field in a conditional that checks the disabled state (the same flag used
to render the checkboxes near the method inputs) and add an opt-out option
(e.g., local include_hidden or opts[:include_hidden] defaulting to true) so
callers can pass include_hidden: false to omit the sentinel; when the picker is
disabled, do not render (or render disabled) the hidden_field so it cannot clear
tags unintentionally. Ensure the change touches the hidden_field invocation and
uses the existing method/disabled variables used for the checkboxes so behavior
is consistent.
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/components/DS/multi_select_tags.html.erb (1)
11-11:⚠️ Potential issue | 🔴 CriticalLine 11: Always-submitted sentinel breaks bulk-edit “preserve tags” semantics.
When enabled, this hidden field always posts
tag_ids, so bulk updates cannot represent “untouched tags” and can clear tags unintentionally.💡 Proposed fix
# app/components/DS/multi_select_tags.rb - attr_reader :form, :method, :tags, :selected_ids, :label, :disabled, :input_options, :dropdown_label + attr_reader :form, :method, :tags, :selected_ids, :label, :disabled, :input_options, :dropdown_label, :include_hidden - def initialize(form:, method:, tags:, selected_ids: [], label: nil, disabled: false, input_options: {}, dropdown_label: nil) + def initialize(form:, method:, tags:, selected_ids: [], label: nil, disabled: false, input_options: {}, dropdown_label: nil, include_hidden: true) `@form` = form `@method` = method `@tags` = tags `@selected_ids` = Array(selected_ids).map(&:to_s) `@selected_id_lookup` = `@selected_ids.each_with_object`({}) { |id, memo| memo[id] = true } `@label` = label `@disabled` = disabled `@input_options` = input_options `@dropdown_label` = dropdown_label || I18n.t("helpers.select.default_label") + `@include_hidden` = include_hidden end# app/components/DS/multi_select_tags.html.erb - <%= form.hidden_field method, multiple: true, value: "", disabled: disabled %> + <% if include_hidden %> + <%= form.hidden_field method, multiple: true, value: "", disabled: disabled %> + <% end %># app/views/transactions/bulk_updates/new.html.erb <%= render DS::MultiSelectTags.new( form: form, method: :tag_ids, tags: Current.family.tags.alphabetically, selected_ids: [], + include_hidden: false, label: t(".tags_label") ) %>Based on learnings, “omitting the tag_ids key entirely … preserves existing tags; sending tag_ids: [""] … should clear tags.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/DS/multi_select_tags.html.erb` at line 11, The always-present hidden sentinel field (the form.hidden_field call using method, value: "", multiple: true) causes tag_ids to always be submitted and breaks bulk-edit "preserve tags" semantics; update the template to stop rendering that hidden field unconditionally — only render the sentinel when you explicitly want to submit an empty tag list (clear tags) or when not in bulk-edit/preserve mode. Concretely, wrap the form.hidden_field(method, multiple: true, value: "", disabled: disabled) in a conditional that checks a flag (e.g., bulk_edit_mode or preserve_tags_flag) and omit the hidden field when preserving tags so the tag_ids key is not sent; when you do need to clear tags, explicitly render tag_ids: [""] instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/transactions/searches/filters/_tag_filter.html.erb`:
- Around line 18-24: Extract the view logic computing tag_color into a view
helper (e.g., TagHelper#display_color or TagPresenter#color_for) that returns
tag.color.presence || Tag::UNCATEGORIZED_COLOR, make the helper available to the
partial, and replace the inline assignment in the partial with a single call to
that helper (use the helper name you add) so the template only references the
helper result and no longer contains domain logic.
---
Duplicate comments:
In `@app/components/DS/multi_select_tags.html.erb`:
- Line 11: The always-present hidden sentinel field (the form.hidden_field call
using method, value: "", multiple: true) causes tag_ids to always be submitted
and breaks bulk-edit "preserve tags" semantics; update the template to stop
rendering that hidden field unconditionally — only render the sentinel when you
explicitly want to submit an empty tag list (clear tags) or when not in
bulk-edit/preserve mode. Concretely, wrap the form.hidden_field(method,
multiple: true, value: "", disabled: disabled) in a conditional that checks a
flag (e.g., bulk_edit_mode or preserve_tags_flag) and omit the hidden field when
preserving tags so the tag_ids key is not sent; when you do need to clear tags,
explicitly render tag_ids: [""] instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 408a28b4-252f-4e5f-b0d4-7321a5aabacc
📒 Files selected for processing (9)
app/components/DS/multi_select_tags.html.erbapp/components/DS/multi_select_tags.rbapp/javascript/controllers/tag_picker_controller.jsapp/views/transactions/_form.html.erbapp/views/transactions/bulk_updates/new.html.erbapp/views/transactions/searches/filters/_tag_filter.html.erbapp/views/transactions/show.html.erbconfig/locales/defaults/en.ymlconfig/locales/views/transactions/en.yml
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/components/DS/multi_select_tags.rb (2)
17-20: Consider adding a brief comment explaining the ID sanitization logic.The
gsubchain correctly handles nested attribute names (e.g.,transaction[tag_ids]→transaction_tag_ids), but this pattern isn't immediately obvious. A brief inline comment would help future maintainers understand the transformation.📝 Suggested documentation
def checkbox_id(tag) + # Sanitize nested attribute names: "foo[bar][baz]" → "foo_bar_baz" scope = form.object_name.to_s.gsub(/\]\[|\[|\]/, "_").gsub(/_+\z/, "") "#{scope}_#{method}_#{tag.id}" end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/DS/multi_select_tags.rb` around lines 17 - 20, Add a concise inline comment above the checkbox_id method explaining the ID sanitization: describe that form.object_name.to_s is transformed with gsub(/\]\[|\[|\]/, "_") to replace bracketed/nested attribute separators with underscores and gsub(/_+\z/, "") to trim trailing underscores, producing a safe DOM id combined with method and tag.id; reference the checkbox_id method, form.object_name, method, and tag.id in the comment so future maintainers immediately understand the transformation.
30-38: Slight inconsistency in color-mix percentages between this component and the tag filter partial.The
selected_chip_styleuses 14%/28% while the tag filter partial uses 10%/20%. If these represent different visual states (selected vs option), this is fine. However, if they should match, consider aligning the values or extracting constants.
Location Background Border selected_chip_style14% 28% option_chip_style10% 20% _tag_filter.html.erb10% 20% 🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/DS/multi_select_tags.rb` around lines 30 - 38, The two helper methods selected_chip_style and option_chip_style use different color-mix percentages (14%/28% vs 10%/20%) which differs from _tag_filter.html.erb; decide whether selected chips should visually differ or match the tag filter and then either align the percentages across selected_chip_style, option_chip_style, and the tag filter partial to the chosen values or extract shared constants (e.g., CHIP_BG_PCT, CHIP_BORDER_PCT) and reference them in both methods and the partial so the values are consistent and easy to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/components/DS/multi_select_tags.rb`:
- Around line 17-20: Add a concise inline comment above the checkbox_id method
explaining the ID sanitization: describe that form.object_name.to_s is
transformed with gsub(/\]\[|\[|\]/, "_") to replace bracketed/nested attribute
separators with underscores and gsub(/_+\z/, "") to trim trailing underscores,
producing a safe DOM id combined with method and tag.id; reference the
checkbox_id method, form.object_name, method, and tag.id in the comment so
future maintainers immediately understand the transformation.
- Around line 30-38: The two helper methods selected_chip_style and
option_chip_style use different color-mix percentages (14%/28% vs 10%/20%) which
differs from _tag_filter.html.erb; decide whether selected chips should visually
differ or match the tag filter and then either align the percentages across
selected_chip_style, option_chip_style, and the tag filter partial to the chosen
values or extract shared constants (e.g., CHIP_BG_PCT, CHIP_BORDER_PCT) and
reference them in both methods and the partial so the values are consistent and
easy to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88d8e2d2-84ca-4aab-9a7d-c86bb86521a3
📒 Files selected for processing (3)
app/components/DS/multi_select_tags.rbapp/helpers/application_helper.rbapp/views/transactions/searches/filters/_tag_filter.html.erb
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
|
Great job! 👏 This is something I wanted to implement as well, to improve the multi-select UI. A few thoughts from my side:
|
|
Let me know if you want to address any of the improvements @alessiocappa suggested. 🙏 |
|
Can you resolve the merge conflict, @IamTaoChen? |
|
This PR overlaps significantly with #1719 (inline tag creation + search in txn form), which is contributor-verified, pr-verified, and further along in review. Recommend closing this in favour of #1719 to avoid merging duplicate solutions for the same UX problem. Generated by Claude Code |
There was a problem hiding this comment.
Two real bugs:
selected_ids: @entry.entryable&.tag_idsreaches outside thefields_forblock to@entrydirectly. For split children,ef.object != @entry.entryableand selections render against the parent's tags.ef.object&.tag_idsfixes it.- Wiring
disabled: @entry.split_child? || !can_annotate_entry?on the new-transaction form makes the picker permanently disabled in that context.can_annotate_entry?(inapp/controllers/concerns/entryable_resource.rb) reads@entry.account.permission_for(Current.user); on a new entry the account isn't selected yet, so permission resolves tonilanddisabledflips to true. All 15 chips ship at opacity 0.3 and the hidden inputs shipdisabled="disabled"— the user can't set tags from the create flow.
A11y semantics question on <details> inline — distinct from CodeRabbit's <fieldset> suggestion.
Rest inline.
| form: ef, | ||
| method: :tag_ids, | ||
| tags: Current.family.tags.alphabetically, | ||
| selected_ids: @entry.entryable&.tag_ids || [], |
There was a problem hiding this comment.
This reaches outside the ef.fields_for :entryable block to @entry directly. For split children, ef.object != @entry.entryable — selections render against the parent's tags instead of the child's. ef.object&.tag_ids || [] would do it. Same fix in show.html.erb.
| </div> | ||
|
|
||
| <%# Candidate dropdown with inline search filter %> | ||
| <details class="group" data-tag-picker-target="dropdown" data-action="toggle->tag-picker#handleDropdownToggle"> |
There was a problem hiding this comment.
<details>/<summary> is the disclosure pattern, not a combobox or listbox. Screen readers announce "summary, button" with no signal that the inner items are selectable options. CodeRabbit's <fieldset>/<legend> suggestion above is related but addresses a different layer (group labeling for assistive tech) — this is specifically about listbox semantics on the dropdown itself.
Shipping this as a new DS component could mean filing an a11y issue on day one. Two paths: (a) keep <details> but layer role="combobox" on summary + role="listbox" on the inner div + role="option" on each button + aria-selected (you're already paying for the JS); (b) switch to a button-toggle + proper combobox pattern. Open to either.
|
|
||
| // The toggle event fires after the open attribute has changed, so | ||
| // dropdownTarget.open === true means the dropdown just opened. | ||
| handleDropdownToggle() { |
There was a problem hiding this comment.
Keyboard nav: after the dropdown opens, options aren't reachable via keyboard until you Tab through every chip. Could we add ArrowDown/ArrowUp inside the search input to move focus into the option list, then arrow within?
| method: :tag_ids, | ||
| tags: Current.family.tags.alphabetically, | ||
| selected_ids: [], | ||
| label: t(".tags_label") |
There was a problem hiding this comment.
Heads up on partial migration: this PR localizes tags_label, but the adjacent "Category" / "Merchant" labels above are still hardcoded English (pre-existing, not introduced here). Could we either localize all three in this PR or leave all three for a follow-up? Partial migration tends to make the next pass harder.
| max_reached: "Maximum file limit reached (%{count}/%{max}). Delete an existing file to upload another." | ||
| bulk_updates: | ||
| new: | ||
| tags_label: Tags |
There was a problem hiding this comment.
New keys here and the no_results in defaults/en.yml only land in English. The repo convention is to add a key to all 14 locale files in the same PR — non-English users will otherwise see English strings or a runtime translation-missing flash. Even pasting the English string with a TODO works.
| <%= icon("search", class: "absolute inset-y-0 left-2 top-1/2 transform -translate-y-1/2") %> | ||
| </div> | ||
| <div class="my-2" id="list" data-list-filter-target="list"> | ||
| <div class="my-2 flex flex-wrap gap-2" id="list" data-list-filter-target="list"> |
There was a problem hiding this comment.
This is a separate concern from the picker (transactions search sidebar). Mixing scope makes the picker change harder to land — could we split this into a follow-up PR?
| @@ -0,0 +1,40 @@ | |||
| module DS | |||
There was a problem hiding this comment.
No component test or Lookbook preview yet. Other DS::* components have both (DS::Alert, DS::Button). Could we at least add a Lookbook preview so we can hit it during the per-component a11y audit?
| data-tag-name="<%= tag.name %>" | ||
| data-action="click->tag-picker#select" | ||
| <%= "disabled" if is_disabled %>> | ||
| <span class="border text-sm font-medium px-3 py-1 rounded-full inline-flex items-center gap-2" |
There was a problem hiding this comment.
Confirmed visually: in the new-transaction form the picker lands disabled (see the separate comment on the disabled: arg) and all options render at opacity 0.3 together — the chips lose enough contrast on their tinted backgrounds to feel unreadable. Even after the disabled-on-new-form bug is fixed, the legitimate disabled state probably wants something softer. Two alternatives: (a) lift opacity to 0.5–0.6 and add a check mark so the disabled state still communicates "selected, here"; (b) remove already-selected options from the candidate list entirely (the chip group above already shows them). Right now the disabled treatment doesn't make it obvious why an option is greyed out.
| tags: Current.family.tags.alphabetically, | ||
| selected_ids: @entry.entryable&.tag_ids || [], | ||
| label: t(".tags_label"), | ||
| disabled: @entry.split_child? || !can_annotate_entry?, |
There was a problem hiding this comment.
Wiring disabled: @entry.split_child? || !can_annotate_entry? on the new-transaction form (not just show.html.erb) permanently disables the picker in that context. can_annotate_entry? (in app/controllers/concerns/entryable_resource.rb) reads @entry.account.permission_for(Current.user); on a new entry the account isn't picked yet, so the permission resolves to nil, the boolean flips to disabled, and the user can't pick any tag. Confirmed visually — all chips ship with opacity: 0.3 and the hidden checkboxes ship disabled="disabled". The split-child guard makes sense on show.html.erb where @entry is loaded; on the create form it should default to enabled (or guard with @entry.persisted? before invoking the permission check).
|
hi @IamTaoChen @jjmata, some further thoughts (echoes previous review comments/change requests): Deterministic blockers:
Concrete blockers:
Required change:
Validation expected:
|
|
Triage note: @gariasf's May 14 review (CHANGES_REQUESTED) flagged two real bugs that still look unaddressed on the current head ( Generated by Claude Code |
7 participants
2026-04-04.14.06.36.mov
Summary by CodeRabbit
New Features
Refactor
Documentation