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.

Sign up for GitHub

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

Jump to bottom

TW-1369: Support Pinned list actions on mobile #1396

Closed
wants to merge 2 commits into from
Closed

TW-1369: Support Pinned list actions on mobile #1396

wants to merge 2 commits into from

Conversation

imGok
Copy link
Contributor

@imGok imGok commented Jan 24, 2024 •
edited
Loading

Bottom Sheet Demo

Android.Emulator.-.Resizable_Experimental_API_33_5554.2024-02-07.16-29-50.mp4

Bottom Bar Demo

Android.Emulator.-.Resizable_Experimental_API_33_5554.2024-02-07.16-31-36.mp4

Web Overview (Actual)

Since I touched to common code, this is a proof of no regression

Twake.Chat.-.Google.Chrome.2024-02-07.16-36-26.mp4

#1369

@imGok imGok force-pushed the TW-1371-improve-selection-message branch from 49351ab to e61ae6c Compare January 24, 2024 18:35
@imGok imGok force-pushed the TW-1369-support-action-mobile-pinned-list branch from 7801bd0 to fd84301 Compare January 25, 2024 16:02
@github-actions GitHub Actions
Copy link

This PR has been deployed to https://linagora.github.io/twake-on-matrix/1396

@imGok imGok force-pushed the TW-1369-support-action-mobile-pinned-list branch from fd84301 to 18c94e9 Compare January 25, 2024 16:40
@imGok imGok changed the base branch from TW-1371-improve-selection-message to main January 25, 2024 16:40
@imGok imGok changed the title WIP TW-1369: Support Pinned list actions on mobile TW-1369: Support Pinned list actions on mobile Jan 25, 2024
@sherlockvn
Copy link
Contributor

Screenshot 2024-01-26 at 15 01 59 Something is wrong, index -1. ?

@imGok
Copy link
Contributor Author

imGok commented Jan 26, 2024

Screenshot 2024-01-26 at 15 01 59 Something is wrong, index -1. ?

you talk about the code markdown ?

nqhhdev
nqhhdev approved these changes Jan 29, 2024
View reviewed changes
lib/pages/chat/chat_pinned_events/pinned_messages_screen.dart Outdated
builder: (context, selectedEvents, child) {
if (selectedEvents.isEmpty) return child!;
responsiveUtils.isMobileOrTablet(context)
? _BuildPinnedMessagesMobileMenu(controller: controller)
Copy link
Member

Choose a reason for hiding this comment

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

IMO: Should create a new file for _BuildPinnedMessagesMobileMenu widget. We will not use private classes anymore because it is very difficult to write widget tests

Copy link
Member

@nqhhdev nqhhdev Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I mistakenly clicked approve, please check my comments

@hoangdat
Copy link
Member

you talk about the code markdown ?

image

@imGok
Copy link
Contributor Author

imGok commented Jan 29, 2024

you talk about the code markdown ?

image

We don't use htmlMessage widget for this text that's why we don't have the markdown

@sherlockvn
Copy link
Contributor

you talk about the code markdown ?

image

We don't use htmlMessage widget for this text that's why we don't have the markdown

no i mean the -1, there is not pinned message -1

@imGok
Copy link
Contributor Author

imGok commented Jan 30, 2024

you talk about the code markdown ?

image

We don't use htmlMessage widget for this text that's why we don't have the markdown

no i mean the -1, there is not pinned message -1

I see, i'll check

@imGok
Copy link
Contributor Author

imGok commented Jan 30, 2024

Commit 012a9db : increment index for display

OLD

Twake.Chat.-.Google.Chrome.2024-01-30.14-15-59.mp4

NEW

Twake.Chat.-.Google.Chrome.2024-01-30.14-14-54.mp4

nqhhdev
nqhhdev reviewed Jan 30, 2024
View reviewed changes
lib/pages/chat/chat_pinned_events/pinned_messages_menu_web.dart Outdated
icon: Icon(
Icons.close,
color: Theme.of(context).colorScheme.onSurfaceVariant,
size: 20,
Copy link
Member

Choose a reason for hiding this comment

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

move it to style file

@imGok imGok force-pushed the TW-1369-support-action-mobile-pinned-list branch from 012a9db to 8c92209 Compare January 31, 2024 14:56
@hoangdat
Copy link
Member

hoangdat commented Jan 31, 2024 •
edited by imGok
Loading

IMO, it still miss some DoD:

  • When long press a message in pinned list, show a bottom sheet action (unpin, jump, copy, select)
  • Change the UI when selects multiple pinned messages (no need to have delete for now, unpin and cancel is enough)

@imGok
Copy link
Contributor Author

imGok commented Jan 31, 2024

IMO, it still miss some DoD:

  • When long press a message in pinned list, show a bottom sheet action (unpin, jump, copy, select)
  • Change the UI when selects multiple pinned messages (no need to have delete for now, unpin and cancel is enough)

second is done
for the first one I'd need #1390 to be merged first

@imGok imGok force-pushed the TW-1369-support-action-mobile-pinned-list branch from dafe863 to 34fbf81 Compare February 5, 2024 15:55
@imGok imGok force-pushed the TW-1369-support-action-mobile-pinned-list branch from 34fbf81 to 2ba6f0d Compare February 7, 2024 15:30
@imGok imGok force-pushed the TW-1369-support-action-mobile-pinned-list branch from 2ba6f0d to 35c1e06 Compare February 7, 2024 15:33
@hoangdat
Copy link
Member

rework on #1559

@hoangdat hoangdat closed this Mar 10, 2024
@hoangdat hoangdat deleted the TW-1369-support-action-mobile-pinned-list branch May 18, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherlockvn sherlockvn sherlockvn left review comments

@nqhhdev nqhhdev nqhhdev approved these changes

@Te-Z Te-Z Te-Z approved these changes

@hoangdat hoangdat Awaiting requested review from hoangdat hoangdat is a code owner

@dab246 dab246 Awaiting requested review from dab246 dab246 is a code owner

@hieutbui hieutbui Awaiting requested review from hieutbui

@tddang-linagora tddang-linagora Awaiting requested review from tddang-linagora tddang-linagora is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@imGok @sherlockvn @hoangdat @Te-Z @nqhhdev