Skip to content

Implement confirmation dialog for deleting items#110

Draft
elliotwutingfeng wants to merge 8 commits intoSUPERCILEX:masterfrom
elliotwutingfeng:dev
Draft

Implement confirmation dialog for deleting items#110
elliotwutingfeng wants to merge 8 commits intoSUPERCILEX:masterfrom
elliotwutingfeng:dev

Conversation

@elliotwutingfeng
Copy link
Contributor

@elliotwutingfeng elliotwutingfeng commented Oct 24, 2022

Addresses #109

Changes

  • Added 2 entry deletion confirmation dialog toggles, one for favorite entries, and the other for non-favorite entries. Both toggles are enabled by default.

Help wanted

  • Fixing the language locales
  • Phrasing of the confirmation dialog messages; should we put the entry contents in the dialog message? Currently it looks like this, could be prettier:
    image

@SUPERCILEX
Copy link
Owner

Can you extract the readme changes into another PR? I'd like to accept that right away.

@elliotwutingfeng
Copy link
Contributor Author

Can you extract the readme changes into another PR? I'd like to accept that right away.

Done. There is still the locale which has not been updated yet.

) {
const next = entry.next;
this._removeEntry(entry, true);
this._confirmRemoveEntry(entry, true);
Copy link
Owner

Choose a reason for hiding this comment

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

This is not something caused by a user action, undo.

last.text.startsWith(text))
) {
this._removeEntry(last, true);
this._confirmRemoveEntry(last, true);
Copy link
Owner

Choose a reason for hiding this comment

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

Undo


_deleteEntryAndRestoreLatest(entry) {
this._removeEntry(entry, true, true);
this._confirmRemoveEntry(entry, true, true);
Copy link
Owner

Choose a reason for hiding this comment

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

Not correct either because this shouldn't trigger a confirmation:

this._deleteEntryAndRestoreLatest(this.currentlySelectedEntry),

I think you need to take in an optional confirm param.

</description>
</key>

<key name="confirm-remove-favorite" type="b">
Copy link
Owner

Choose a reason for hiding this comment

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

These settings need to be supported in code (prefs.js).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants