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

feat: enhance markdown parsing to support bold text with spaces\n #8200

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ovgodd
Copy link

@Ovgodd Ovgodd commented Sep 3, 2024

Summary

This merge request aims to trigger bold text when a user adds a space or more after the last text letter and before the last two asterisks. For example, "**TEXT **" will be displayed as "TEXT".
I have added a TransformBoldText function that uses a regular expression to check if there is a space after the last text letter. If a space is detected, the function returns the text in bold.

Ticket Link

Checklist

.

Device Information

This PR was tested on:

Screenshots

Release Note

feat: enhance markdown parsing to support bold text with spaces\n

@mm-cloud-bot mm-cloud-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 3, 2024
@mattermost-build
Copy link
Contributor

Hello @Ovgodd,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@saturninoabril saturninoabril added the Build Apps for PR Build the mobile app for iOS and Android to test label Sep 4, 2024
@hmhealey
Copy link
Member

hmhealey commented Sep 9, 2024

Thanks for the PR, @Ovgodd!

Could you explain a bit about why you're wanting to add this? Are you wanting it to behave like some other app that uses Markdown?

I'm hesitant to accept this because this would go against the CommonMark spec. We're trying to follow that more closely here than we do in the web app because that makes us more consistent with other apps which also use Markdown

@Ovgodd
Copy link
Author

Ovgodd commented Sep 11, 2024

Hello !

thanks for your feedback,
i suggested this because users on a project where i worked expressed the need to have this feature, as it would improve their user experience. but i do understand that it doesn't necessarily respect markdown standards.

@rahimrahman
Copy link
Contributor

@hmhealey I actually meant to post this and somehow forgotten 😢 !

I noticed that on the desktop and web app:

space-bold-desktop-app

vs

mobile:

IMG_3121

@hmhealey I think the changes by @Ovgodd solve this particular problem, but since we're using commommark.js to transform markdown, I wonder if there's anything special on the web that we're doing to catch the space(s) between the double asterisks/strong that we can do on mobile. You were working on commonmark.js recently and I thought your guidance would be extremely valuable.

@hmhealey
Copy link
Member

I appreciate the feedback from you and your users as well. I think in this case though, I'd rather follow the standard because it makes it much easier for people to come into MM and know what to expect because it works the same as GitHub and other places that support CommonMark. We used to make this sort of change in the past, but it often caused bugs and let users crash each others' apps which is another reason we've stopped doing it.

Regarding this being different from the web app, that's actually because the web app uses a different Markdown library which predates the mobile app's and a fork from before that library followed the CommonMark spec. I've wanted to have the web app use the same Markdown code as the mobile app for years now (https://mattermost.atlassian.net/browse/MM-10003) because it's spec-compliant, easier to work with, and I've got some ideas for cool features to build with it, but it's something I've never gotten to.

Even though we're not accepting these changes, thanks again for your work on this though and thanks especially for your interest in helping to improve Mattermost!

@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Apps for PR Build the mobile app for iOS and Android to test Contributor kind/feature Categorizes issue or PR as related to a new feature. Lifecycle/1:stale release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants