-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[MM-53810] Add mobile markdown support for highlighting without notifications without configuration ability #7663
Conversation
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.
LGTM 👍
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.
In general looks good. Just marking it as request changes
for the number of comments, but I don't think there is anything major in them.
export function highlightWithoutNotification(ast: Node, highlightKeys: HighlightWithoutNotificationKey[]) { | ||
const walker = ast.walker(); | ||
|
||
const patterns = highlightKeysToPatterns(highlightKeys); |
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.
How often is this executed? I guess only once per markdown render, but wanted to make sure. It has a couple of loops inside and wanted to make sure it is not too complex.
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 per the markdown logic this is executed on each text node of a post
SelfHostedProducts: { | ||
STARTER: 'starter', | ||
PROFESSIONAL: 'professional', | ||
ENTERPRISE: 'enterprise', | ||
}, |
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.
These are the same as the SKU_SHORT_NAMEs, right? Is there any strong reason not to use those?
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.
Although it's the same as the SKU and the same as the Licence, the key is different, so I have added it.
/update-branch |
Building app in separate branch. |
/update-branch |
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 work! Looks good to me
Building app in separate branch. |
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.
Tested this on iOS 17.0.3, Android 11.0
- able to see words and phrases highlighted.
- behaviour is same on web and mobile.
- able to edit on web and see the changes on mobile
- able to see words highlighted in different theme
…ications without configuration ability (mattermost#7663)
…ications without configuration ability (mattermost#7663)
Summary
Ticket Link
https://mattermost.atlassian.net/browse/MM-53810
Checklist
Includes text changes and localization file updatesDevice Information
This PR was tested on:
Screenshots
Release Note