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

copy msg link to clipboard #5285

Merged
merged 3 commits into from
Mar 8, 2021
Merged

copy msg link to clipboard #5285

merged 3 commits into from
Mar 8, 2021

Conversation

jakobroehrl
Copy link
Contributor

Signed-off-by: Jakob Röhrl jakob.roehrl@web.de
solves: #5097

image

@nickvergessen
Copy link
Member

So with more and more actions (delete, deck-integration, reply-private, copy link) I think the most used action (reply) is getting moved to far away as you always need to open the ... first.
cc @ma12-co @jancborchardt any clever ideas how we can make replying quick again?

@PVince81
Copy link
Member

PVince81 commented Mar 2, 2021

So with more and more actions (delete, deck-integration, reply-private, copy link) I think the most used action (reply) is getting moved to far away as you always need to open the ... first.
cc @ma12-co @jancborchardt any clever ideas how we can make replying quick again?

we probably need a second button just for replying.
and the three dots menu directly after that

@jancborchardt
Copy link
Member

So with more and more actions (delete, deck-integration, reply-private, copy link) I think the most used action (reply) is getting moved to far away as you always need to open the ... first.

@nickvergessen Thought exactly the same, yep.

we probably need a second button just for replying.
and the three dots menu directly after that

@PVince81 exactly that – just like we do in files too, or e.g. in the left navigation of calendar, in both cases where "Share" is a direct action. In this case "Reply" will be the direct action, and everything else is details.

@marcoambrosini
Copy link
Member

Agree, should we create a separate ticket for this?

@nickvergessen
Copy link
Member

yeah, it shouldn't block a "random" pr adding another action

@nickvergessen
Copy link
Member

Reported #5297

Signed-off-by: Jakob Röhrl <jakob.roehrl@web.de>
@jakobroehrl
Copy link
Contributor Author

added it to the new action menu:
image

@PVince81
Copy link
Member

PVince81 commented Mar 4, 2021

nice!

I'm starting to wonder if we should start to add some separators in there, maybe to separate the hard-coded actions from the registered ones.

@ma12-co any thoughts on this ? (see above screenshot)

@marcoambrosini
Copy link
Member

marcoambrosini commented Mar 4, 2021

Yep, we could have something like:

talk actions
---
other apps actions 
---
delete 

@PVince81
Copy link
Member

PVince81 commented Mar 4, 2021

@jakobroehrl do you want to take care of the separators or is this something for another PR ?

note: there are like more actions coming

@jakobroehrl
Copy link
Contributor Author

@jakobroehrl do you want to take care of the separators or is this something for another PR ?

note: there are like more actions coming

I will try the seperators here in this PR

Could you say me the actions, maybe I can help with the easy ones?

@PVince81
Copy link
Member

PVince81 commented Mar 5, 2021

Could you say me the actions, maybe I can help with the easy ones?

I have a PR that will bring "mark as unread" here later on: #3825.
I thought there were other already open PRs with more actions, not sure. In any case if we add separators as @ma12-co we can insert the new actions in the places that make sense between/after/before the separators.

Signed-off-by: Jakob Röhrl <jakob.roehrl@web.de>
@jakobroehrl
Copy link
Contributor Author

supernice :)
Let's merge it?
image

@jakobroehrl
Copy link
Contributor Author

I have a PR that will bring "mark as unread" here later on: #3825.

@PVince81 Maybe I can do this? Please ping me if it's easy and when I can start

@PVince81
Copy link
Member

PVince81 commented Mar 5, 2021

I have a PR that will bring "mark as unread" here later on: #3825.

@PVince81 Maybe I can do this? Please ping me if it's easy and when I can start

@jakobroehrl I already created the action there, thanks :-)

The only thing you can do right now is add the separators as @ma12-co advised. I'd say put them in this PR here.
Future work can then decide where to put new actions.

@PVince81
Copy link
Member

PVince81 commented Mar 5, 2021

okay, you added already. let me review!

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

Nice work @jakobroehrl :)
One question, I'm not able to copy links of my own messages, I think it would be nice to be able to do so too. Is there a particular reason why this wasn't implemented?

As a side note, I also think that we should use var(--color-primary-light) for the fading feedback on the target message to give some more emphasis and differentiate it from the messages hover/focus feedbacks.

@jakobroehrl
Copy link
Contributor Author

Nice work @jakobroehrl :)
One question, I'm not able to copy links of my own messages, I think it would be nice to be able to do so too. Is there a particular reason why this wasn't implemented?

Thanks! :)
That's a bug, fixed it.

As a side note, I also think that we should use var(--color-primary-light) for the fading feedback on the target message to give some more emphasis and differentiate it from the messages hover/focus feedbacks.

Sorry, what do you mean here? What exactly should I change?

@marcoambrosini
Copy link
Member

That's a bug, fixed it.

Noice :)

Sorry, what do you mean here? What exactly should I change?

Basically those 3 variables in the animation into var(--color-primary-light)

https://github.com/nextcloud/spreed/blob/master/src/components/MessagesList/MessagesGroup/Message/Message.vue#L750

Signed-off-by: Jakob Röhrl <jakob.roehrl@web.de>
@nickvergessen nickvergessen merged commit b74ed27 into master Mar 8, 2021
@nickvergessen nickvergessen deleted the enh/copyMsgLink branch March 8, 2021 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants