Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add option to disable markdown #5356

Closed
wants to merge 9 commits into from
Closed

Add option to disable markdown #5356

wants to merge 9 commits into from

Conversation

resynth1943
Copy link
Contributor

Fixes element-hq/element-meta#1189

Open to UI changes. This is a baseline; it works rather well, but we need tests.

Signed-off-by: Resynth resynth1943@tutanota.com

Signed-off-by: Resynth <resynth1943@tutanota.com>
Signed-off-by: Resynth <resynth1943@tutanota.com>
Signed-off-by: Resynth <resynth1943@tutanota.com>
@resynth1943
Copy link
Contributor Author

image

src/settings/Settings.ts Outdated Show resolved Hide resolved
@@ -74,7 +75,9 @@ export function createMessageContent(model, permalinkCreator, replyToEvent) {
msgtype: isEmote ? "m.emote" : "m.text",
body: body,
};
const formattedBody = htmlSerializeIfNeeded(model, {forceHTML: !!replyToEvent});
const formattedBody = SettingsStore.getValue("enableMarkdown") &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm relatively sure moving this to createMessageContent would break Markdown-formatted messages by others, hence its inception(s) in the composer components.

Copy link
Member

Choose a reason for hiding this comment

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

Doubtful given messages are not markdown over the wire, the sender converts them into HTML and the other ends just renders it.

@resynth1943
Copy link
Contributor Author

I'm really not sure what to do about the I18N errors. I'll leave those files alone until I get advice from an Element developer.

Signed-off-by: Resynth <resynth1943@tutanota.com>
Signed-off-by: Resynth <resynth1943@tutanota.com>
Signed-off-by: Resynth <resynth1943@tutanota.com>
Signed-off-by: Resynth <resynth1943@tutanota.com>
@resynth1943 resynth1943 marked this pull request as ready for review October 26, 2020 00:33
@t3chguy t3chguy requested review from a team October 27, 2020 12:59
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Code generally seems sane - thanks for the contribution!

@aaronraimist
Copy link
Contributor

You could also fix element-hq/element-web#3728 at the same time by making the option contain a link to CommonMark

Like

Format your messages with Markdown

@resynth1943
Copy link
Contributor Author

@aaronraimist wait, I didn't do that?

I swear, I was just gonna do that here. Must've slipped my mind, sorry.
I'll whip open ol' Codium now and get crackin' then!

@nadonomy
Copy link
Contributor

@resynth1943 thanks for the contribution! 2 q's:

  1. Does this PR now require users to opt in to enable markdown?
  2. Please can you include a screenshot of any UI changes (e.g. extra settings) to aid review.

@nadonomy
Copy link
Contributor

@resynth1943 bumping this last comment, please let us know updates on those points or else this PR will likely be triaged/closed soon.

@nunoperalta
Copy link

nunoperalta commented Feb 11, 2021

Hey,
I'm not the developer, but I'm hoping to get this released asap, as I keep wishing for this PR since looooong ago.

But, based on the code, it seems that Markdown will be enabled by default, as per: https://github.com/matrix-org/matrix-react-sdk/pull/5356/files/a5e4a03fa8b2133effcc70211f3f8c83c1a1d701#diff-b4f775f8ca52f8aa92695986fffd693ec8428bf69e27576721ad0d5b62a4c637R294

As for screenshots, these are the ones I can find:

image

image

@jryans
Copy link
Collaborator

jryans commented Feb 17, 2021

I believe @resynth1943 is no longer actively contributing, so let's close this for now. Someone else could pick this up in a new PR if they'd like to.

@jryans jryans closed this Feb 17, 2021
@SimonBrandner
Copy link
Contributor

Related to #16304 (comment)

@nunoperalta
Copy link

@nadonomy @jryans Why can't this PR simply be approved and tested?... and if anything needs to be changed/fixed, someone can look into that.

This is probably the ONLY reason I'm considering moving away from Element/Matrix, as I keep having inconveniences of markdown messing up all my spacing between paragraphs, or transforming "+" into bullets, and all that crap I don't need.

I've been ANXIOUS to get this feature merged to Matrix...

Closing a PR just because the owner didn't provide a screenshot.
Really not happy with this, sorry.

@SimonBrandner
Copy link
Contributor

SimonBrandner commented Feb 17, 2021

Closing a PR just because the owner didn't provide a screenshot.

@nunoperalta, the PR was closed because the author doesn't seem to be active anymore. If someone wants to pick this up they can

@nunoperalta
Copy link

@nunoperalta, the PR was closed because the author doesn't seem to be active anymore. If someone wants to pick this up they can

Still don't think it was a good reason to discard the work done for this.
This would be a very valuable change for Matrix, and the work is all done already. Just needs to be tested.

@SimonBrandner
Copy link
Contributor

Still don't think it was a good reason to discard the work done for this.

The work hasn't been discarded, anyone can go and pick this up. The team just has other priorities

@t3chguy
Copy link
Member

t3chguy commented Feb 17, 2021

and the work is all done already. Just needs to be tested.

And it needs further iterations, for one an /md command being added and probably a round of design.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warn users if markdown is going to kick-in on their message, and give a nice UI for disabling it if desired
8 participants