-
Notifications
You must be signed in to change notification settings - Fork 46
feat: move wallet item options to a dropdown menu #768
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
base: beeman/ui-confirm-controlled
Are you sure you want to change the base?
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
samui-wallet-web | 4b35c5b | Commit Preview URL Branch Preview URL |
Dec 14 2025, 03:23 PM |
1e85561 to
4b35c5b
Compare
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.
Caution
Changes requested ❌
Reviewed everything up to 1e85561 in 2 minutes and 21 seconds. Click for details.
- Reviewed
170lines of code in5files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/settings/src/ui/settings-ui-wallet-menu.tsx:31
- Draft comment:
For better accessibility, add an ARIA label (e.g., aria-label="Wallet options menu") to the menu button since it only displays an icon. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is an accessibility suggestion for a new file. The comment is about adding an aria-label to an icon-only button, which is a legitimate accessibility concern. However, I need to consider: 1) The rules say "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." 2) Accessibility is important, but this could be considered a "nice to have" rather than a critical issue. 3) The rules also say "Do NOT comment on anything that would be obviously caught by the build" - accessibility linting might catch this. 4) The comment is actionable and specific. 5) However, the rules emphasize not making comments unless there's a CLEAR code change required, and accessibility improvements might be considered optional enhancements rather than required fixes. This could be a legitimate accessibility improvement that makes the code better. The comment is specific and actionable. It's possible that the project doesn't have automated accessibility linting that would catch this, making the manual review valuable. The suggestion improves code quality in a measurable way. While accessibility is important, the rules explicitly state not to make comments unless there's clearly a code change REQUIRED. This appears to be a quality improvement suggestion rather than a bug fix. Additionally, if the project has accessibility linting tools, this would be caught automatically. The rules prioritize only commenting on clear issues, not best practices or enhancements. This comment should be deleted. While it's a valid accessibility suggestion, it falls into the category of "nice to have" improvements rather than required code changes. The rules emphasize only commenting when there's a clear, required code change, not for best practice suggestions or quality improvements.
Workflow ID: wflow_HdvmLmDXw5ggoqPB
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| {t(($) => $.actionEditWallet)} | ||
| </Link> | ||
| </DropdownMenuItem> | ||
| <DropdownMenuItem className="cursor-pointer" onClick={() => setOpenDelete((value) => !value)}> |
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.
Instead of toggling the delete modal state with setOpenDelete((value) => !value), consider setting it explicitly (setOpenDelete(true)) to ensure the modal reliably opens.
| <DropdownMenuItem className="cursor-pointer" onClick={() => setOpenDelete((value) => !value)}> | |
| <DropdownMenuItem className="cursor-pointer" onClick={() => setOpenDelete(true)}> |
| <DropdownMenuItem className="cursor-pointer" onClick={() => setOpenDelete((value) => !value)}> | ||
| {t(($) => $.actionDeleteWallet)} | ||
| </DropdownMenuItem> | ||
| <DropdownMenuItem className="cursor-pointer" onClick={() => setOpenExport((value) => !value)}> |
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.
Similarly, rather than toggling openExport with setOpenExport((value) => !value), use setOpenExport(true) so that the export sheet always opens as expected.
| <DropdownMenuItem className="cursor-pointer" onClick={() => setOpenExport((value) => !value)}> | |
| <DropdownMenuItem className="cursor-pointer" onClick={() => setOpenExport(true)}> |
| <SettingsUiExportWalletMnemonicSheet open={openExport} setOpen={setOpenExport} wallet={wallet} /> | ||
| <UiConfirm | ||
| action={async () => await deleteItem(wallet)} | ||
| actionLabel="Delete" |
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 delete confirmation's actionLabel is hardcoded as "Delete". Consider using a localized string with t() to support internationalization.
| actionLabel="Delete" | |
| actionLabel={t(($) => $.actionDeleteWallet)} |
BundleMonFiles added (3)
Files updated (10)
Unchanged files (88)
Total files change +23.96KB +2.91% Groups updated (3)
Final result: ✅ View report in BundleMon website ➡️ |
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.
Caution
Changes requested ❌
Reviewed 4b35c5b in 1 minute and 46 seconds. Click for details.
- Reviewed
236lines of code in7files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_lncGAlOdR18JNoZE
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| <SettingsUiExportWalletMnemonicSheet open={openExport} setOpen={setOpenExport} wallet={wallet} /> | ||
| <UiConfirm |
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.
Consider localizing the static texts in the UiConfirm component (actionLabel, description, title) instead of using hardcoded English strings for consistency with other translations.
Description
Moves the 'edit/delete/export' option to a dropdown menu.
Part of #736
Screenshots / Video
Important
Consolidates wallet item actions into a dropdown menu, removing individual buttons and updating translations.
SettingsUiWalletListItem.SettingsUiWalletMenucomponent to handle dropdown actions.SettingsUiExportWalletMnemonicandSettingsUiWalletDeleteConfirmcomponents.actionDeleteWalletandexportMnemonickeys toen/settings.jsonandes/settings.json.resources.d.tsto include new translation keys.This description was created by
for 4b35c5b. You can customize this summary. It will automatically update as commits are pushed.