Skip to content
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

FEATURE: Edit bookmark reminders from post and explicit delete button #9455

Merged
merged 8 commits into from
Apr 20, 2020

Conversation

martin-brennan
Copy link
Contributor

@martin-brennan martin-brennan commented Apr 17, 2020

There is now an explicit "Delete Bookmark" button in the edit modal:

image

A confirmation is shown before deleting

image

Along with this, when the bookmarked post icon is clicked the modal is now shown instead of just deleting the bookmark. Also, the "Delete Bookmark" button from the user bookmark list now confirms the action.

Add a d d shortcut in the modal to delete the bookmark.

* when created return new bookmark id in response
* serialize bookmark name and reminder at for post for
  editing post bookmark
* return whether topic_bookmarked is true from bookmark manager
* move old after delete code for post into an afterDelete callback
@martin-brennan
Copy link
Contributor Author

@coding-horror moved your comment here, I had to close that other PR because weird stuff was happening

I don’t think you need the word “bookmark” in the delete button? Then the
copy is not unique and easier to localize. Heck maybe just use the glyph...

Will do!

this.deleting = true;
bootbox.confirm(I18n.t("bookmarks.confirm_delete"), result => {
if (result) {
this.closeWithoutSaving = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this variable didn't start in this commit but as a general note - if you are only using a variable in a particular controller, and nothing is computed based on it, we prefer to underscore them as private. For example this._closeWithoutSaving.

Absolutely a minor thing, though and just something to keep in mind for the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eviltrout cool I was talking to Joffrey about this as well RE: functions because I wasn't sure, so I will make sure to do it from now on :)

@martin-brennan martin-brennan merged commit 344ef52 into master Apr 20, 2020
@martin-brennan martin-brennan deleted the feature/add-explicit-delete-bookmark branch April 20, 2020 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants