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

mentions #105

Merged
merged 1 commit into from
Jun 15, 2020
Merged

mentions #105

merged 1 commit into from
Jun 15, 2020

Conversation

yenda
Copy link
Contributor

@yenda yenda commented Apr 29, 2020

No description provided.

@oskarth oskarth self-requested a review April 30, 2020 10:13
@yenda yenda self-assigned this May 4, 2020
@oskarth oskarth requested a review from decanus May 6, 2020 06:51
title: Mentions
nav_order: 2
has_children: true
permalink: /specs/draft
Copy link
Contributor

Choose a reason for hiding this comment

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

Number

Also, is this a draft or is it stable? I.e. is it describing what is live already?


## Usage

To mention a user, one would type a message containing `@0xpk` where `pk` is the public key of the user to be mentioned.
Copy link
Contributor

Choose a reason for hiding this comment

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

Refer to relevant spec for account here

`status-go` parses the markdown in the messages and detects mentions that follow the pattern described above. This is done in [`parser.inline.go`](https://github.com/status-im/markdown/blob/b9fe921681227b1dace4b56364e15edb3b698308/parser/inline.go#L68). When mentions are found they are added as an element to the message that is passed to the client with the following format:

```
{:type "mention" :literal "0x......"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Refer to how this relates to e.g. PAYLOAD spec


## Future work

The current implementation is cumbersome as it forces users to input the whole public key of the mentioned user. Implementing auto-completion of mentions would make the feature more appealing to users. It would allow them to mention other users in the chat by usernames or random usernames, which would be replaced by the pk before sending the message
Copy link
Contributor

Choose a reason for hiding this comment

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

I initially thought it autocompleted and worked with ENS names too

Does that mean this is very much a WIP feature and should remain a draft for the time being?

cc @andremedeiros @hesterbruikman

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes actually mentions is my next assignment so I'm going to update this draft afterward

@oskarth oskarth requested a review from andremedeiros May 6, 2020 07:15

The `status-react` client uses specific styling for the following elements: `code`, `emph`, `strong`, `link`, `status-tag` and `mention`.
`mentions` will be displayed in a color that sets it apart from regular text and tapping it will open the chat with the mentioned user.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copyright

Copyright and related rights waived via CC0.

@oskarth
Copy link
Contributor

oskarth commented May 14, 2020

Ping @yenda

@oskarth
Copy link
Contributor

oskarth commented May 25, 2020

Ping @yenda whats the state of this? It'd be great if we can merge it and create follow up issues where things still need to be done.

@oskarth
Copy link
Contributor

oskarth commented Jun 8, 2020

@status-im/status-core @status-im/status-protocol review

Copy link
Member

@cammellos cammellos left a comment

Choose a reason for hiding this comment

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

Currently I think we are overspeccing things,
for example: tapping it should open the chat with the mentioned user is completely an UI concern.
I would not have a separate document for mentions, probably one describing the markdown supported and how is parsed is enough.
Having said that, if we are ok with the amount of information, this looks ok to me.

docs/stable/14-mentions.md Outdated Show resolved Hide resolved
Comment on lines 22 to 23
`status-go` currently parses the following markdown elements: `code`, `emph`, `strong`, `link`, `status-tag` and `mention`.
`mentions` should be displayed in a color that sets it apart from regular text and tapping it should open the chat with the mentioned user.
Copy link
Member

Choose a reason for hiding this comment

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

Is there a dedicated section to markdown? Are the other markdown elements documented?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`status-go` currently parses the following markdown elements: `code`, `emph`, `strong`, `link`, `status-tag` and `mention`.
`mentions` should be displayed in a color that sets it apart from regular text and tapping it should open the chat with the mentioned user.
`status-go` currently parses the following markdown elements: `code`, `emph`, `strong`, `link`, `status-tag` and `mention`.
A `mention` tag SHOULD be displayed in a color that sets it apart from regular text and tapping it should open the chat with the mentioned user.

Copy link
Member

Choose a reason for hiding this comment

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

If there isn't a dedicated section documenting status-go markdown can we at least create an issue to track that markdown needs to be documented?

There is no mentions tag, as far as I can see there is only the mention tag.

On reflection this section should probably be a UI implementation recommendation.

Example:

  • This specification RECOMMENDs that mention tags:
    • are displayed in a colour that sets it apart from regular text
    • open the chat with the mentioned pk when tapped or clicked

Comment on lines 22 to 23
`status-go` currently parses the following markdown elements: `code`, `emph`, `strong`, `link`, `status-tag` and `mention`.
`mentions` should be displayed in a color that sets it apart from regular text and tapping it should open the chat with the mentioned user.
Copy link
Member

Choose a reason for hiding this comment

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

and tapping it should open the chat with the mentioned user.

This seems to be how it is implemented now, but should this be some thing that is detailed as a specification for all implementations?


## Usage

To mention a user, one would type a message containing `@0xpk` where `pk` is the public key of the [user account](https://specs.status.im/spec/2) to be mentioned. The message is sent as a [regular message with `TEXT_PLAIN` content type](https://specs.status.im/spec/6#content-types).
Copy link
Member

Choose a reason for hiding this comment

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

Would the user manually enter the pk manually or does the application enter the pk in place of a pseudonym?

Copy link
Member

Choose a reason for hiding this comment

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

Clarification is needed on this section

  • Who is one? Is this the user or the application in the background?
  • Recommendations need to be given for how the one entity enters the pk.
    • Examples
      • This specification RECOMMENDs that the application does not require the user to enter the entire pk.
      • This specification RECOMMENDs that the application allows the user to create a mention by typing @ followed by the related ENS or 3-word pseudonym.
      • This specification RECOMMENDs that the application provides the user auto-completion functionality to create a mention.

@oskarth
Copy link
Contributor

oskarth commented Jun 9, 2020

Currently I think we are overspeccing things,
for example: tapping it should open the chat with the mentioned user is completely an UI concern.
I would not have a separate document for mentions, probably one describing the markdown supported and how is parsed is enough.
Having said that, if we are ok with the amount of information, this looks ok to me.

I think that's a fair point. Perhaps it makes sense to amend existing spec and describe "mentions" / ens username there, without going into detail on how the UI should behave?

@oskarth oskarth requested a review from Samyoul June 10, 2020 09:13
@Samyoul
Copy link
Member

Samyoul commented Jun 10, 2020

Hey @yenda , the fundamentals of this spec are solid. There are just a few points of clarification I've highlighted that I feel need attention. To be helpful I've written out more details to help with the implementation of these clarifications.


Also perhaps this spec could be renamed as 14-markdown.md if there isn't a dedicated markdown spec, and use the current work as the starting point for a more robust document. If it is too much work to implement this change in the scope of this PR, perhaps we could create an issue to track the need for additional documentation of markdown usage.

@yenda
Copy link
Contributor Author

yenda commented Jun 15, 2020

@cammellos @Samyoul I took both of your comments into consideration and decided to move the spec to the payloads spec, and remove the implementation details, so that it now consists only of the bare minimum + UX recommendations. Ok to merge?

Copy link
Member

@Samyoul Samyoul left a comment

Choose a reason for hiding this comment

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

Thank you @yenda , I appreciate your willingness to move this documentation. I can understand why you placed this into its own document at the beginning, and I think you've made a good choice to move this information into the payloads spec.

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.

None yet

4 participants