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

Mobile drafts #8280

Open
wants to merge 56 commits into
base: main
Choose a base branch
from
Open

Mobile drafts #8280

wants to merge 56 commits into from

Conversation

Rajat-Dabade
Copy link
Contributor

@Rajat-Dabade Rajat-Dabade commented Oct 22, 2024

Summary

Separate tab for Drafts in mobile.

Ticket Link

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

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.
  • Have run E2E tests by adding label E2E iOS tests for PR.

Device Information

This PR was tested on: iOS version 17.5 (Both on iPhone and ipad)

Screenshots

Release Note

Added a feature for having a separate tab for Drafts in mobile. 

Note:
Will cover all the test case for this PR in separate PR.

@Rajat-Dabade
Copy link
Contributor Author

/update-branch

@Rajat-Dabade Rajat-Dabade marked this pull request as ready for review November 5, 2024 12:34
@Rajat-Dabade Rajat-Dabade requested review from larkox and enahum November 5, 2024 12:34
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! Some comments.

app/actions/local/draft.ts Outdated Show resolved Hide resolved
app/components/draft/draft.tsx Outdated Show resolved Hide resolved
app/components/draft/draft.tsx Outdated Show resolved Hide resolved
app/components/draft/draft_post/draft_files/index.ts Outdated Show resolved Hide resolved
app/components/draft/draft_post/draft_files/index.ts Outdated Show resolved Hide resolved
app/components/post_draft/post_input/post_input.tsx Outdated Show resolved Hide resolved
app/queries/servers/drafts.ts Outdated Show resolved Hide resolved
app/screens/global_threads/global_threads.tsx Outdated Show resolved Hide resolved
assets/base/i18n/en.json Outdated Show resolved Hide resolved
assets/base/i18n/en.json Outdated Show resolved Hide resolved
@Rajat-Dabade Rajat-Dabade added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester 2: UX Review Requires review by a UX Designer labels Nov 7, 2024
@yasserfaraazkhan yasserfaraazkhan added Build Apps for PR Build the mobile app for iOS and Android to test E2E iOS tests for PR Run iOS E2E Detox tests labels Nov 8, 2024
@Rajat-Dabade
Copy link
Contributor Author

/update-branch

@Rajat-Dabade Rajat-Dabade requested a review from larkox November 13, 2024 10:46
@yasserfaraazkhan yasserfaraazkhan added E2E iOS tests for PR Run iOS E2E Detox tests and removed E2E iOS tests for PR Run iOS E2E Detox tests labels Nov 14, 2024
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.

Thank you @Rajat-Dabade

Most flows around

  • Creating Draft in DM, GM, Channel, Threads, Private Chanel, under deleted thread ✅
  • Sending Threads, Editing threads work as expected. ✅

There couple of flow I wanted to bring to your notice

  1. On the Draft Screen, if we hit on Android's back button (not the back arrow of MM app) the app closes instead of going back to channels list page

  2. Once all drafts are sent out from the draft page, we have an empty page looking like below. would it be nice to add a 'No drafts at the moment' icon, like in web app. @asaadmahmood thoughts?

Screenshot_20241118-143836

@Rajat-Dabade
Copy link
Contributor Author

Thank you @yasserfaraazkhan for reviewing the code.

On the Draft Screen, if we hit on Android's back button (not the back arrow of MM app) the app closes instead of going back to channels list page

Let me check on this.

Once all drafts are sent out from the draft page, we have an empty page looking like below. would it be nice to add a 'No drafts at the moment' icon, like in web app. @asaadmahmood thoughts?

I remember having an empty screen for the draft view, let me add it.

@Rajat-Dabade
Copy link
Contributor Author

@yasserfaraazkhan

  • On the Draft Screen, if we hit on Android's back button (not the back arrow of MM app) the app closes instead of going back to channels list page ✅
  • Once all drafts are sent out from the draft page, we have an empty page looking like below. would it be nice to add a 'No drafts at the moment' icon, like in web app. @asaadmahmood thoughts? ✅

Can you please review again? Thanks.

@Rajat-Dabade Rajat-Dabade added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Nov 18, 2024
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.

Thank you @Rajat-Dabade
just 1 last change, in below flow

Scenarios 1:
Steps:

  • From Draft page, Edit a draft. Verify you are taken to the Channel/DM/GM
  • Edit the Draft in the Channel
  • Click on the < back Icon on the channel

This is taking user to Draft page. should it take user to Channels List page?

Scenarios 2:

With above steps, if we edit draft message (without sending it) and Click back button, the draft message is not updated.

Screen.Recording.2024-11-19.at.12.16.41.AM.mov

Scenario 3:

  • The User group Name is shown like below
Screenshot 2024-11-19 at 12 29 22 AM

on web app it show as below

image

@Rajat-Dabade Rajat-Dabade mentioned this pull request Nov 20, 2024
5 tasks
Copy link

@asaadmahmood asaadmahmood left a comment

Choose a reason for hiding this comment

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

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! Many comments, but mostly nitpicks.

app/components/channel_info/channel_info.tsx Outdated Show resolved Hide resolved
app/components/channel_info/avatar/index.tsx Outdated Show resolved Hide resolved
app/queries/servers/drafts.ts Show resolved Hide resolved
app/queries/servers/drafts.ts Outdated Show resolved Hide resolved
app/screens/global_drafts/index.tsx Show resolved Hide resolved
app/screens/global_drafts/index.tsx Show resolved Hide resolved
app/components/drafts_buttton/index.tsx Outdated Show resolved Hide resolved
app/components/drafts_buttton/index.tsx Outdated Show resolved Hide resolved
@Rajat-Dabade
Copy link
Contributor Author

@larkox I’ve addressed all the review comments. Thank you for highlighting the changes. They all make perfect sense. Could you please re-review it whenever you have some time? Much appreciated! 🙌

@Rajat-Dabade Rajat-Dabade requested a review from larkox December 5, 2024 12:58
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! LGTM

@Rajat-Dabade
Copy link
Contributor Author

just 1 last change, in below flow
Scenarios 1:
Steps:
From Draft page, Edit a draft. Verify you are taken to the Channel/DM/GM
Edit the Draft in the Channel
Click on the < back Icon on the channel
This is taking user to Draft page. should it take user to Channels List page?

@yasserfaraazkhan, I have fixed this can you please retest it? Thank you.

@yasserfaraazkhan yasserfaraazkhan added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test E2E iOS tests for PR Run iOS E2E Detox tests labels Dec 10, 2024
@yasserfaraazkhan
Copy link
Contributor

/update-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.

Verified on android and iOS device

  • Drafts created in Channel/DM/GM
  • send/edit/delete actions on drafts
  • swipe left on draft to delete
  • drafts on public channel and then convert
  • Remove server that has drafts and add it back to see no drafts are present
  • Theme applied on the page.
  • Drafts on multiple teams
  • Draft count updated when we leave channel, archive channel

Few observations,

  • On android where we can see channels list page for fraction of a second, when we click on a particular Draft
az_recorder_20241211_150748.mp4
  • Theme issues when Device is in Dark mode.

Steps:

  1. select Onxy (dark theme)
  2. create Draft in channel ,
  3. change to Denim theme
  4. go to channel and see the theme is applied
  5. go to Draft, long press draft, Click on Edit

Actual: we see Dark theme section

Screenshot_20241211-151829
Screenshot_20241211-151834

@Rajat-Dabade Rajat-Dabade added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Dec 11, 2024
@Rajat-Dabade Rajat-Dabade added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Dec 11, 2024
Copy link
Contributor

@enahum enahum left a comment

Choose a reason for hiding this comment

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

Overall looks good, great job!

I have added a few comments


export async function parseMarkdownImages(markdown: string, imageMetadata: Dictionary<PostImage | undefined>) {
let match;
const imageRegex = /!\[.*?\]\((https:\/\/[^\s)]+)\)/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the url is only with http this will not match. Is that what we want?

while ((match = imageRegex.exec(markdown)) !== null) {
const imageUrl = match[1];
// eslint-disable-next-line no-await-in-loop
imageMetadata[imageUrl] = await getImageMetadata(imageUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks prone to race

const serverUrl = useServerUrl();

let uri = '';
if (author) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the prop, author should always be defined, something I'm missing?

import {typography} from '@utils/typography';
import {getUserTimezone} from '@utils/user';

import CompassIcon from '../compass_icon';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import CompassIcon from '../compass_icon';
import CompassIcon from '@components/compass_icon';

Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the folder does not make a lot of sense to me


return (
<View
style={{flex: 1}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Set this in the stylesheet

size={ICON_SIZE}
color={changeOpacity(theme.centerChannelColor, 0.56)}
/>
<Text style={style.title}>{intl.formatMessage({id: 'draft.options.send.title', defaultMessage: 'Edit draft'})}</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

Use FormattedText


import React from 'react';
import {useIntl} from 'react-intl';
import {Image, Text, View} from 'react-native';
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhere else you are using Image from expo-image for the same purpose, which makes sense, change it here

onClose: () => void;
}

const logo = require('@assets/images/emojis/swipe.png');
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps change the name of the variable so that it makes sense instead of logo


import type {AvailableScreens} from '@typings/screens/navigation';

const edges: Edge[] = ['left', 'right'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not include bottom? Is it added elsewhere? I see top in the styles but not bottom. Or is it that we don't want it at all?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core commiter 2: UX Review Requires review by a UX Designer 3: QA Review Requires review by a QA tester Build Apps for PR Build the mobile app for iOS and Android to test release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants