Skip to content

Comments

Add remove link button#1962

Merged
juliusknorr merged 1 commit intomasterfrom
update/floating-marking-bar
Nov 24, 2021
Merged

Add remove link button#1962
juliusknorr merged 1 commit intomasterfrom
update/floating-marking-bar

Conversation

@luka-nextcloud
Copy link
Contributor

@luka-nextcloud luka-nextcloud commented Nov 23, 2021 •
edited
Loading

Signed-off-by: Luka Trovic [email protected]

Summary

Added "Remove link" action in the bar when a link is set.

image

@julien-nc julien-nc added this to the Nextcloud 24 milestone Nov 24, 2021
@julien-nc julien-nc self-requested a review November 24, 2021 10:08
julien-nc
julien-nc reviewed Nov 24, 2021
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Nice and clean!

Removing a link only affects the selected text so if a sub part of the link text is selected and one hits "remove link", the rest is still a link. This makes sense but I wonder if users would expect the whole link to be removed.

What do you think?

How do you build the scripts? In your branch, when doing npm ci; npm run build I get 18 different files in js/.

@luka-nextcloud
Copy link
Contributor Author

luka-nextcloud commented Nov 24, 2021

@eneiluj @juliushaertl
I guess, it would make more sense to remove the whole link.

I just ran npm run build.

@luka-nextcloud
Copy link
Contributor Author

luka-nextcloud commented Nov 24, 2021

@eneiluj @juliushaertl
I've just checked again, it works exactly the same as the update link option. I think we need to update the behavior of the update link option as well or leave it as it is.

@julien-nc
Copy link
Member

julien-nc commented Nov 24, 2021

I think we need to update the behavior of the update link option as well or leave it as it is.

@luka-nextcloud I think it can be done in a second time as it's out of scope. Let's leave it like this 👍

I would approve this PR but let's wait for a confirmation of @juliushaertl about the built files.

@juliusknorr
Copy link
Member

juliusknorr commented Nov 24, 2021

As discussed via chat, might need another rebase against latest master ;)

juliusknorr
juliusknorr approved these changes Nov 24, 2021
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Let's get that in when CI passes 👍

@juliusknorr
Copy link
Member

juliusknorr commented Nov 24, 2021

@luka-nextcloud Please make sure to add the original ticket where this was mentioned to the pull request description, so we have a back reference there. If it is an overview ticket just mention that it contibutes to #ticketnumber or something similar :)

@luka-nextcloud
add button to remove link
ae26b8a 
Signed-off-by: Luka Trovic <[email protected]>
@luka-nextcloud luka-nextcloud force-pushed the update/floating-marking-bar branch from 32d234a to ae26b8a Compare November 24, 2021 17:09
@luka-nextcloud luka-nextcloud mentioned this pull request Nov 24, 2021
Closed
39 tasks
@juliusknorr juliusknorr merged commit 427c653 into master Nov 24, 2021
@delete-merged-branch delete-merged-branch bot deleted the update/floating-marking-bar branch November 24, 2021 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@julien-nc julien-nc Awaiting requested review from julien-nc

Assignees

No one assigned

Labels

3. to review enhancement New feature or request format: markdown

Projects

None yet

Milestone

Nextcloud 24

Development

Successfully merging this pull request may close these issues.

✨ Text app design review

3 participants

@luka-nextcloud @julien-nc @juliusknorr