-
Notifications
You must be signed in to change notification settings - Fork 23
Amend mention message html output #716
Amend mention message html output #716
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #716 +/- ##
============================================
+ Coverage 88.38% 90.07% +1.69%
============================================
Files 146 81 -65
Lines 17003 14261 -2742
Branches 789 0 -789
============================================
- Hits 15028 12846 -2182
+ Misses 1785 1415 -370
+ Partials 190 0 -190
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
// use the display text or the presence of a `data-mention-type => at-room` attribute to decide the mention type | ||
let new_node = if text == "@room".into() | ||
|| attributes.iter().any(|(k, v)| { | ||
k == &S::from("data-mention-type") && v == &S::from("at-room") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's an @room
mention if and only if there is an occurrence of @room
in the text making this condition redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great - I wanted to think that was the case but wasn't sure. Updated in e8bd655
@@ -77,7 +77,16 @@ where | |||
let (start, end) = self.safe_selection(); | |||
let range = self.state.dom.find_range(start, end); | |||
|
|||
let new_node = DomNode::new_mention(url, text, attributes); | |||
// use the display text or the presence of a `data-mention-type => at-room` attribute to decide the mention type | |||
let new_node = if text == "@room".into() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| attributes.iter().any(|(k, v)| { | ||
k == &S::from("data-mention-type") && v == &S::from("at-room") | ||
}) { | ||
DomNode::new_at_room_mention(attributes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this function is able to create an @room
mention, we might want to reconsider the API, and either:
- make the
url
argument optional, or - accept an enum (
AtRoom
|Matrix(text, url)
- the actual types might come from Add Matrix mentions utils [wip] #695).
Alternatively, we could add a new function so that we have:
do_insert_mention(text, url) // with mandatory args
do_insert_at_room_mention()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd be tempted to go a different way and have this function call DomNode::new_mention
with url, display_text, attributes as we currently have and then have MentionNode::new
create either a room, user, or at-room mention as appropriate.
What do you think to that solution?
If you're happy with me adding a note as the work may be further informed by the WIP mentions utils work (which is what I'm picking up next), I'll do that and address in that subsequent PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have this function call DomNode::new_mention with url, display_text, attributes as we currently have and then have MentionNode::new create either a room, user, or at-room mention as appropriate.
Yeah that makes sense. I still think the API could be more self explanatory in terms of URL being optional, and some documentation of that behaviour if we do keep a single function for both. But happy for that to be addressed in a later PR
attributes.push(("href".into(), "#".into())); // designates a placeholder link in html | ||
attributes.push(("contenteditable".into(), "false".into())); | ||
|
||
self.fmt_tag_open(tag, formatter, &Some(attributes)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this tag be <span>
(or <mention>
), rather than <a>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the time being keeping everything as <a>
- there's a separate issue for moving to a custom html tag type here to try and keep these PRs small and digestible
} else { | ||
DomNode::new_mention(url, text, attributes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When trying to insert an invalid mention, should the editor still do that? I'm wondering if it could return an error result and/or fall back to DomNode::new_link()
.
It might not be possible to do without being able to parse mentions but could be worth adding a note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question, and for now I believe we don't have a way of telling if it's valid or not. Will add a note to handle that (after parsing gives us that ability).
SonarCloud Quality Gate failed. |
This PR makes it so that when you translate a mention to "message format" html then the following rules are applied:
- if it's an at-room mention, only output the raw text
@room
- if it's a room or user mention, output the mention as
<a href="...">display_text</a>
It also does a little bit of TS work so that we can use the lib function in TS code and makes a couple of tweaks to the example app for testing.