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

[MM-53810] Add mobile markdown support for highlighting without notifications without configuration ability #7663

Merged
merged 18 commits into from
Nov 23, 2023

Conversation

M-ZubairAhmed
Copy link
Member

@M-ZubairAhmed M-ZubairAhmed commented Nov 14, 2023

Summary

Ticket Link

https://mattermost.atlassian.net/browse/MM-53810

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.

Device Information

This PR was tested on:

  • Apple iPhone 15 - iOS 15.0
  • Google Pixel 6 - Android 13.0

Screenshots

Release Note

Added a new feature for highlighting without notification without the ability to configure.

@M-ZubairAhmed M-ZubairAhmed added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Nov 14, 2023
@M-ZubairAhmed M-ZubairAhmed changed the title [MM-53810] Add highlight support for highlight without notifications to markdown in mobile [MM-53810] Add mobile markdown support for highlighting without notifications without configuration ability Nov 14, 2023
@M-ZubairAhmed M-ZubairAhmed marked this pull request as ready for review November 20, 2023 20:08
Copy link
Member

@harshilsharma63 harshilsharma63 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@larkox larkox left a 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 changesfor the number of comments, but I don't think there is anything major in them.

app/components/markdown/transform.test.ts Show resolved Hide resolved
export function highlightWithoutNotification(ast: Node, highlightKeys: HighlightWithoutNotificationKey[]) {
const walker = ast.walker();

const patterns = highlightKeysToPatterns(highlightKeys);
Copy link
Contributor

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.

Copy link
Member Author

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

app/components/post_list/post/body/message/message.tsx Outdated Show resolved Hide resolved
Comment on lines +12 to +16
SelfHostedProducts: {
STARTER: 'starter',
PROFESSIONAL: 'professional',
ENTERPRISE: 'enterprise',
},
Copy link
Contributor

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?

Copy link
Member Author

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.

app/queries/servers/system.ts Outdated Show resolved Hide resolved
app/queries/servers/system.ts Outdated Show resolved Hide resolved
app/queries/servers/system.ts Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
app/queries/servers/system.ts Outdated Show resolved Hide resolved
@M-ZubairAhmed M-ZubairAhmed requested a review from larkox November 21, 2023 17:33
@M-ZubairAhmed
Copy link
Member Author

/update-branch

@M-ZubairAhmed M-ZubairAhmed added the Build Apps for PR Build the mobile app for iOS and Android to test label Nov 22, 2023
@mattermost-build mattermost-build removed the Build Apps for PR Build the mobile app for iOS and Android to test label Nov 22, 2023
@mattermost-build
Copy link
Contributor

Building app in separate branch.

@M-ZubairAhmed
Copy link
Member Author

/update-branch

Copy link
Contributor

@larkox larkox left a 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

@M-ZubairAhmed M-ZubairAhmed added Build Apps for PR Build the mobile app for iOS and Android to test and removed 2: Dev Review Requires review by a core commiter labels Nov 22, 2023
@mattermost-build mattermost-build removed the Build Apps for PR Build the mobile app for iOS and Android to test label Nov 22, 2023
@mattermost-build
Copy link
Contributor

Building app in separate branch.

Copy link
Contributor

@yasserfaraazkhan yasserfaraazkhan left a 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

@enahum enahum added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Nov 23, 2023
@enahum enahum merged commit 4b5489c into main Nov 23, 2023
4 checks passed
@enahum enahum deleted the MM-53810 branch November 23, 2023 09:11
@amyblais amyblais added this to the v2.12.0 milestone Nov 23, 2023
fewva pushed a commit to fewva/mattermost-mobile that referenced this pull request Jan 12, 2024
cyrusjc pushed a commit to cyrusjc/mattermost-mobile that referenced this pull request May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants