Skip to content

fix(enable_banking): migrate select_bank dialog to DS::Pill and DS::Link#1997

Open
glorydavid03023 wants to merge 3 commits into
we-promise:mainfrom
glorydavid03023:fix/enable-banking-select-bank-ds-drift
Open

fix(enable_banking): migrate select_bank dialog to DS::Pill and DS::Link#1997
glorydavid03023 wants to merge 3 commits into
we-promise:mainfrom
glorydavid03023:fix/enable-banking-select-bank-ds-drift

Conversation

@glorydavid03023
Copy link
Copy Markdown

@glorydavid03023 glorydavid03023 commented May 25, 2026

Summary

  • Bank list Beta badge: hand-rolled span -> DS::Pill (tone: :warning, badge mode)
  • Cancel control: hand-rolled link_to -> DS::Link (variant: :secondary), preserving turbo frame + dialog close behavior

Issues

Not duplicated

Test plan

  • Settings -> Providers -> Enable Banking -> connect -> bank picker shows Beta pills
  • Cancel closes dialog and returns to providers
  • Light and dark mode contrast on Beta pill and Cancel link

Summary by CodeRabbit

  • Style
    • Improved visual consistency in the bank selection modal: beta badges now use a standardized pill with translated “Beta” label and warning tone, and the cancel control was updated to a consistent design-system link that closes the dialog.

Review Change Stack

@superagent-security superagent-security Bot added the contributor:verified Contributor passed trust analysis. label May 25, 2026
@superagent-security superagent-security Bot added the pr:verified PR passed security analysis. label May 25, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 30204d77-3b94-47ff-ab26-f4cfb78ec6fc

📥 Commits

Reviewing files that changed from the base of the PR and between 2815275 and 043fd5c.

📒 Files selected for processing (1)
  • app/views/enable_banking_items/select_bank.html.erb

📝 Walkthrough

Walkthrough

The select_bank modal view now renders the bank “Beta” indicator using DS::Pill (translated label, warning tone) and replaces the modal cancel link_to with DS::Link.new that navigates to settings_providers_path and closes the dialog.

Changes

View Component Modernization

Layer / File(s) Summary
Design system component migration
app/views/enable_banking_items/select_bank.html.erb
Beta indicator switches from utility-class <span> to DS::Pill with translated label and warning tone. Cancel link switches from link_to to DS::Link.new, preserving navigation to settings_providers_path and the DS--dialog#close action.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • we-promise/sure#1859: Updates modal cancel controls to use DS::Link instead of plain link_to in a different view.
  • we-promise/sure#1902: Adds/relies on DS::Pill tone/label behavior used when switching beta indicators to DS::Pill.
  • we-promise/sure#1939: Migrates inline badge markup to DS::Pill in other ERB views.

Suggested reviewers

  • jjmata

Poem

🐰 I hopped into the modal bright,
Replaced a span with a pill of light,
A linked goodbye that skips and sings,
Translated words on gentle wings,
Consistency for every sight.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main changes: migrating the select_bank dialog to use DS::Pill and DS::Link components, which matches the file changes and PR objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

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

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: 2815275d30

ℹ️ 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".

Comment thread app/views/enable_banking_items/select_bank.html.erb
Replace hand-rolled Beta pill and secondary cancel link with DS::Pill and DS::Link on the bank picker dialog.

Refs we-promise#1971
@glorydavid03023 glorydavid03023 force-pushed the fix/enable-banking-select-bank-ds-drift branch from 525131e to 043fd5c Compare May 26, 2026 00:44
Copy link
Copy Markdown
Collaborator

jjmata commented May 26, 2026

Clean substitution. DS::Pill with tone: :warning and marker: false is the right call for the beta badge (warning tone, no leading dot). DS::Link with variant: :secondary preserves the visual intent of the cancel control. The data attributes (turbo_frame: "_top", action: "DS--dialog#close") are correctly threaded through to DS::Link.


Generated by Claude Code

@jjmata jjmata requested a review from gariasf May 26, 2026 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis.

Development

Successfully merging this pull request may close these issues.

3 participants