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

Add ability to delete and re-draft a comment with no replies + markdown infos + unicode emojis #3046

Conversation

kimsible
Copy link
Contributor

@kimsible kimsible commented Aug 7, 2020

  • Hide mention of deleted comments without replies ;
  • New delete and re-draft action on a comment only on comments without replies ;
  • Move delete, delete and re-draft actions on options menu ;
  • Add icons on delete, delete and re-draft and report actions in options menu ;
  • Rename Reply value of add comment button to Comment only for comment main thread textarea ;
  • Add Markdown help icon :
  • Add uni-code emojis native display (depending on Operating System) https://apps.timwhitlock.info/emoji/tables/unicode.

Comments-markdown

@kimsible kimsible changed the title Add ability to delete and re-draft a comment, hide deleted comments with no replies and show markdown infos Add ability to delete and re-draft a comment with no replies and show markdown infos Aug 7, 2020
@rigelk
Copy link
Collaborator

rigelk commented Aug 7, 2020

Nice! I like the markdown icon but at the same time I'm not too sure about it's position. Usually this is right aligned in the input, and greyed like the input border.

@kimsible
Copy link
Contributor Author

kimsible commented Aug 7, 2020

Nice! I like the markdown icon but at the same time I'm not too sure about it's position. Usually this is right aligned in the input, and greyed like the input border.

Yes, but we have the extend native icon of textarea on the right.
We also can do as GiThub does, but we loose the one line textarea and the UI becomes more complex :
image

@kimsible kimsible force-pushed the feat/hide-deleted-comment-when-no-children branch from fe02077 to e4c91ba Compare August 7, 2020 22:18
@kimsible
Copy link
Contributor Author

kimsible commented Aug 7, 2020

@rigelk, was it what you thought ?
markdown-on-right

@kimsible
Copy link
Contributor Author

kimsible commented Aug 8, 2020

The last commit adds unicode emojis to markdown (depending on Operating System it will have a different display https://apps.timwhitlock.info/emoji/tables/unicode)
emoji

@kimsible kimsible changed the title Add ability to delete and re-draft a comment with no replies and show markdown infos Add ability to delete and re-draft a comment with no replies + markdown infos + unicode emojis Aug 8, 2020
@rigelk rigelk added UI non-trivial UI changes, that might need discussion Component: Comments labels Aug 8, 2020
@rigelk
Copy link
Collaborator

rigelk commented Aug 8, 2020

@kimsible
Copy link
Contributor Author

kimsible commented Aug 8, 2020

@kimsible that is what I though, yes :)

Could you please fix https://github.com/Chocobozzz/PeerTube/pull/3046/checks?check_run_id=961331548#step:6:126?

Thanks, I'm working on it.

For the emojis, I don't know if we must include an emoji collection into PeerTube, in the best way, yes, because some people do not have every one on their operating system (especially on desktop linux and windows).

What do we do ?

@rigelk
Copy link
Collaborator

rigelk commented Aug 8, 2020

For the emojis, I don't know if we must include an emoji collection into PeerTube, in the best way, yes, because some people do not have every one on their operating system (especially on desktop linux and windows).

What do we do ?

I'm inclined to look at what Mastodon does, where they adopted a full emoji management interface that is highly extensible. Until then, only including a light set like markdown-it-emoji provides sounds like a possible option (+25kB for the full version or +7kB for the light version sounds reasonable).

It's not like PeerTube is only focusing on text and thus very reliant on emojis, but emojis have their place too in the comment palette (i.e. an issue has already considered video reactions - a much heavier development - which proves there is interest for richer expression forms to react to videos).

@kimsible
Copy link
Contributor Author

kimsible commented Aug 8, 2020

only including a light set like markdown-it-emoji provides sounds like a possible option (+25kB for the full version or +7kB for the light version sounds reasonable).

Yes, the light version sounds reasonable thanks.

It's not like PeerTube is only focusing on text and thus very reliant on emojis, but emojis have their place too in the comment palette (i.e. an issue has already considered video reactions - a much heavier development - which proves there is interest for richer expression forms to react to videos).

I think you meant this PR #3026 ?

@rigelk
Copy link
Collaborator

rigelk commented Aug 8, 2020

@kimsible both of them actually, though they are just issues, not PRs.

@kimsible
Copy link
Contributor Author

kimsible commented Aug 8, 2020

@kimsible both of them actually, though they are just issues, not PRs.

Ok @rigelk , so you mean adding a Popover with emojis ?

Otherwise, I'm not sure (since I'm not an expert of Typescript) but light version emoji of markdown-it seems to not be implemented to @types/markdown-it-emoji https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/markdown-it-emoji/index.d.ts, the commonjs way is require('markdown-it-emoji/light'), do you know how to do with Typescript ?

@rigelk
Copy link
Collaborator

rigelk commented Aug 8, 2020

@kimsible both of them actually, though they are just issues, not PRs.

Ok @rigelk , so you mean adding a Popover with emojis ?

no, I didn't mean anything like this. The issue I quoted was merely an example of how text is arguably not the only response type we should consider, as an argument in favor of including emojis (since you asked if we should include emojis).

Back on your current error, leave it at the typical require for the moment. We could always write types, but for now it's more work than it's worth.

@kimsible
Copy link
Contributor Author

kimsible commented Aug 9, 2020

The last commits uses Commonjs require to include markdown-it emoji plugin and to get light emoji list.
It also refactor the Markdown Pophover :
image

image

@kimsible kimsible force-pushed the feat/hide-deleted-comment-when-no-children branch 3 times, most recently from 3652611 to 02b3ac6 Compare August 9, 2020 15:00
client/package.json Outdated Show resolved Hide resolved
client/yarn.lock Outdated Show resolved Hide resolved
@kimsible kimsible force-pushed the feat/hide-deleted-comment-when-no-children branch from d5c6cfe to a101280 Compare August 12, 2020 11:33
@kimsible
Copy link
Contributor Author

Thanks for the review @Chocobozzz, I've fixed / refactored all your comments.

@kimsible
Copy link
Contributor Author

kimsible commented Aug 14, 2020

@Chocobozzz maybe this PR should have been merged before changing localization system, we need to re-pass on all files.

Except this.i18n('') to $localize``, templates with i18n attributes must be changed too ?

@rigelk
Copy link
Collaborator

rigelk commented Aug 14, 2020

@kimsible templates are not touched, just their associated components logic as the I18n service model is replaced.

@kimsible
Copy link
Contributor Author

kimsible commented Aug 14, 2020

@kimsible templates are not touched, just their associated components logic as the I18n service model is replaced.

Thanks @rigelk, ok, in this PR we have some i18n-title and i18n tags, so we leave them as they are ?

@rigelk
Copy link
Collaborator

rigelk commented Aug 14, 2020

Yes, since templates are not affected.

@Chocobozzz
Copy link
Owner

Chocobozzz commented Aug 14, 2020

I'll merge and fix conflicts of this PR myself.

@kimsible kimsible force-pushed the feat/hide-deleted-comment-when-no-children branch from d3464a2 to 2a7eab0 Compare August 14, 2020 12:35
@kimsible
Copy link
Contributor Author

I'll merge and fix conflicts of this PR myself.

Thanks @Chocobozzz, I've already merged it but maybe you would like to check if I missed something.

@kimsible kimsible force-pushed the feat/hide-deleted-comment-when-no-children branch from 2a7eab0 to cd28662 Compare August 14, 2020 12:36
@Chocobozzz Chocobozzz merged commit da188b9 into Chocobozzz:develop Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Comments UI non-trivial UI changes, that might need discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants