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

Allow styling text in composer when selecting it with native actions #14249

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

qfrank
Copy link
Contributor

@qfrank qfrank commented Oct 27, 2022

close(partially) #14137

note: this PR doesn't contain corresponding implementation for Ios yet, someone's help would appreciated :)

@status-im-auto
Copy link
Member

status-im-auto commented Oct 27, 2022

Jenkins Builds

Click to see older builds (48)
Commit #️⃣ Finished (UTC) Duration Platform Result
c0fc672 #2 2022-10-27 13:52:09 ~4 min tests 📄log
✔️ c0fc672 #2 2022-10-27 13:56:25 ~8 min android-e2e 📦apk 📲
✔️ c0fc672 #2 2022-10-27 13:56:26 ~8 min android 📦apk 📲
✔️ c0fc672 #2 2022-10-27 14:02:47 ~14 min ios 📦ipa 📲
3dfc0b4 #3 2022-10-28 01:50:22 ~12 sec ios 📄log
3dfc0b4 #3 2022-10-28 01:50:22 ~12 sec android-e2e 📄log
3dfc0b4 #3 2022-10-28 01:50:34 ~24 sec android 📄log
3dfc0b4 #3 2022-10-28 01:50:38 ~23 sec tests 📄log
3f45341 #4 2022-10-28 02:01:23 ~1 min tests 📄log
✔️ 3f45341 #4 2022-10-28 02:06:43 ~7 min android-e2e 📦apk 📲
✔️ 3f45341 #4 2022-10-28 02:07:25 ~7 min android 📦apk 📲
✔️ 3f45341 #4 2022-10-28 02:13:12 ~13 min ios 📦ipa 📲
1dc0fd4 #5 2022-11-09 00:52:13 ~1 min tests 📄log
✔️ 1dc0fd4 #5 2022-11-09 00:58:10 ~7 min android 📦apk 📲
✔️ 1dc0fd4 #5 2022-11-09 00:58:11 ~7 min android-e2e 📦apk 📲
✔️ 1dc0fd4 #5 2022-11-09 01:02:38 ~12 min ios 📦ipa 📲
ee26cbd #6 2022-11-09 09:03:53 ~2 min tests 📄log
✔️ ee26cbd #6 2022-11-09 09:10:32 ~8 min android-e2e 📦apk 📲
✔️ ee26cbd #6 2022-11-09 09:10:38 ~8 min android 📦apk 📲
✔️ ee26cbd #6 2022-11-09 09:16:49 ~15 min ios 📦ipa 📲
2407840 #7 2022-11-09 09:44:33 ~2 min tests 📄log
✔️ 2407840 #7 2022-11-09 09:50:56 ~9 min android 📦apk 📲
✔️ 2407840 #7 2022-11-09 09:51:25 ~9 min android-e2e 📦apk 📲
✔️ 2407840 #7 2022-11-09 09:59:13 ~17 min ios 📦ipa 📲
f709cd8 #8 2022-11-09 10:33:26 ~2 min tests 📄log
✔️ f709cd8 #8 2022-11-09 10:40:09 ~9 min android-e2e 📦apk 📲
✔️ f709cd8 #8 2022-11-09 10:42:29 ~11 min android 📦apk 📲
✔️ f709cd8 #8 2022-11-09 10:46:37 ~15 min ios 📦ipa 📲
✔️ 2c69495 #9 2022-11-09 13:04:25 ~1 min tests 📦log
✔️ 2c69495 #9 2022-11-09 13:11:05 ~8 min android-e2e 📦apk 📲
✔️ 2c69495 #9 2022-11-09 13:11:46 ~9 min android 📦apk 📲
✔️ 2c69495 #9 2022-11-09 13:13:56 ~11 min ios 📦ipa 📲
✔️ b525607 #10 2022-11-09 15:26:19 ~4 min tests 📦log
✔️ b525607 #10 2022-11-09 15:31:07 ~9 min android 📦apk 📲
✔️ b525607 #10 2022-11-09 15:31:28 ~10 min android-e2e 📦apk 📲
✔️ b525607 #10 2022-11-09 16:49:34 ~1 hr 28 min ios 📦ipa 📲
✔️ c079525 #11 2022-11-10 01:10:55 ~1 min tests 📦log
✔️ c079525 #11 2022-11-10 01:16:29 ~7 min android-e2e 📦apk 📲
✔️ c079525 #11 2022-11-10 01:19:06 ~10 min android 📦apk 📲
✔️ c079525 #11 2022-11-10 01:22:08 ~13 min ios 📦ipa 📲
✔️ 343bdd4 #12 2022-11-17 11:03:27 ~2 min tests 📦log
✔️ 343bdd4 #12 2022-11-17 11:09:06 ~7 min android 📦apk 📲
✔️ 343bdd4 #12 2022-11-17 11:09:24 ~8 min android-e2e 📦apk 📲
✔️ 343bdd4 #12 2022-11-17 11:13:36 ~12 min ios 📦ipa 📲
✔️ 08f09b6 #14 2022-11-18 11:26:06 ~2 min tests 📦log
✔️ 08f09b6 #14 2022-11-18 11:31:46 ~7 min android-e2e 📦apk 📲
✔️ 08f09b6 #14 2022-11-18 11:32:45 ~8 min android 📦apk 📲
✔️ 08f09b6 #14 2022-11-18 11:38:03 ~14 min ios 📦ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 06f006c #15 2022-11-21 13:28:17 ~2 min tests 📦log
✔️ 06f006c #15 2022-11-21 13:33:45 ~7 min android 📦apk 📲
✔️ 06f006c #15 2022-11-21 13:34:04 ~8 min android-e2e 📦apk 📲
✔️ 06f006c #15 2022-11-21 13:41:14 ~15 min ios 📦ipa 📲
✔️ 5850510 #16 2022-11-23 01:53:40 ~2 min tests 📦log
✔️ 5850510 #16 2022-11-23 01:58:35 ~7 min android-e2e 📦apk 📲
✔️ 5850510 #16 2022-11-23 01:59:02 ~8 min android 📦apk 📲
✔️ 5850510 #16 2022-11-23 02:04:45 ~13 min ios 📦ipa 📲

@qfrank qfrank force-pushed the feature/14137 branch 2 times, most recently from 3dfc0b4 to 3f45341 Compare October 28, 2022 01:59
@qfrank qfrank requested a review from OmarBasem October 31, 2022 02:43
@OmarBasem
Copy link
Contributor

Hey @qfrank, can you please explain why the implementation is done natively? As far as I understand from the designs it does not need to be styled in the composer itself.

@qfrank
Copy link
Contributor Author

qfrank commented Nov 4, 2022

Hey @qfrank, can you please explain why the implementation is done natively? As far as I understand from the designs it does not need to be styled in the composer itself.

As someone else may have same question, i'd like to put this quote here:
"How could you add a custom menu item BIU to the popup menu without native implementation?"

Copy link
Contributor

@ibrkhalil ibrkhalil left a comment

Choose a reason for hiding this comment

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

Is there a way to add unit testing to this functionality ?

@qfrank qfrank requested a review from cammellos November 7, 2022 10:19
@ilmotta
Copy link
Contributor

ilmotta commented Nov 8, 2022

@qfrank, I tried to run the app locally, but unfortunately https://github.com/qfrank/status-react/tree/feature/14137 fails to me with the error below. Even the develop branch in your fork fails to me. To double-check it wasn't a problem on my machine, I rebased upstream/develop onto origin/develop (locally) and everything worked just fine. So, could you please update the fork with the latest changes from status-im/status-mobile develop branch, so that I can test your PR?

node_modules/react-native-blob-util/android/src/main/java/com/ReactNativeBlobUtil/ReactNativeBlobUtilUtils.java:3: error: package com.facebook.react.bridge does not exist
import com.facebook.react.bridge.Arguments;

@qfrank
Copy link
Contributor Author

qfrank commented Nov 9, 2022

@qfrank, I tried to run the app locally, but unfortunately https://github.com/qfrank/status-react/tree/feature/14137 fails to me with the error below. Even the develop branch in your fork fails to me. To double-check it wasn't a problem on my machine, I rebased upstream/develop onto origin/develop (locally) and everything worked just fine. So, could you please update the fork with the latest changes from status-im/status-mobile develop branch, so that I can test your PR?

node_modules/react-native-blob-util/android/src/main/java/com/ReactNativeBlobUtil/ReactNativeBlobUtilUtils.java:3: error: package com.facebook.react.bridge does not exist
import com.facebook.react.bridge.Arguments;

Hi @ilmotta , rebased, enjoy!

@qfrank
Copy link
Contributor Author

qfrank commented Nov 9, 2022

Hi @ibrkhalil , thanks for your review, a lot of align task :), can we leave the align task later to make lint-fix? currently, i'm using IntelliJ with cursive plugin, i've made an auto format with IDE, then make lint-fix, but make lint-fix won't align let or map ATM, i hope we can get this pr merged ASAP. @flexsurfer @cammellos thoughts?

@qfrank
Copy link
Contributor Author

qfrank commented Nov 9, 2022

Is there a way to add unit testing to this functionality ?

i don't think we need here, if u know there is a way, i'd like to hear

@ilmotta
Copy link
Contributor

ilmotta commented Nov 9, 2022

Hi @ilmotta , rebased, enjoy!

Thanks @qfrank. I could successfully run it now.

So I tried in the Android Emulator, and I took two screenshots (below) where we can see the native BIU menu option. The functionality works, but the behavior and implementation is completely different from what I see in Figma (https://www.figma.com/file/wA8Epdki2OWa8Vr067PCNQ/Composer-for-Mobile?node-id=2230%3A116813). A couple important differences:

  • The Figma spec clearly shows the styling should not be applied on selection, i.e. the user is not required to select the text.
  • In the Figma file, I can see the styling BIU buttons are not native. We can even see button icons are part of our design system.
step-01 step-02

"How could you add a custom menu item BIU to the popup menu without native implementation?"

Where's the Figma spec showing we should implement a native BIU option in the popup menu? The image below shows the styling button is not in a popup menu.

Hence I actually don't understand how this PR is implementing what the Figma specs require. Could you help me understand why the mismatch between this PR's implementation and the Figma specs?

@ilmotta
Copy link
Contributor

ilmotta commented Nov 9, 2022

Hi @ibrkhalil , thanks for your review, a lot of align task :), can we leave the align task later to make lint-fix? currently, i'm using IntelliJ with cursive plugin, i've made an auto format with IDE, then make lint-fix, but make lint-fix won't align let or map ATM, i hope we can get this pr merged ASAP. @flexsurfer @cammellos thoughts?

I don't know if it helps you, but this Cursive page says aligning let bindings is supported.
https://cursive-ide.com/userguide/formatting.html

And you're right, make lint-fix never supported aligning map keys and let bindings, so all of us are using what our IDEs/editors support. If/when we start using zprint, then this tool will actually be able to format map keys and let bindings.

@qfrank
Copy link
Contributor Author

qfrank commented Nov 9, 2022

Hi @ilmotta , rebased, enjoy!

Thanks @qfrank. I could successfully run it now.

So I tried in the Android Emulator, and I took two screenshots (below) where we can see the native BIU menu option. The functionality works, but the behavior and implementation is completely different from what I see in Figma (https://www.figma.com/file/wA8Epdki2OWa8Vr067PCNQ/Composer-for-Mobile?node-id=2230%3A116813). A couple important differences:

  • The Figma spec clearly shows the styling should not be applied on selection, i.e. the user is not required to select the text.
  • In the Figma file, I can see the styling BIU buttons are not native. We can even see button icons are part of our design system.

step-01 step-02

"How could you add a custom menu item BIU to the popup menu without native implementation?"

Where's the Figma spec showing we should implement a native BIU option in the popup menu? The image below shows the styling button is not in a popup menu.

Hence I actually don't understand how this PR is implementing what the Figma specs require. Could you help me understand why the mismatch between this PR's implementation and the Figma specs?

Good questions, i had a conversation with petro.et before (i should ask it in public channel ):), let me quote here:

me : when the text get selected, the menu popup which contains menu items like Copy/Paste../BIU etc. Could u explain why we need duplicate it as we have our own in the bottom ? snapshot
petro: so in that particular instance the formatting options are open but it may be that the user selects text when its not. so at the bottom there would be the add image, add reactions, format buttons snapshot , so if the user would select text at this point in time

I guess above would answer the first difference?
On the second difference, i've discussed with @cammellos before that i don't have to implement the bottom part ATM. it would be a separate issue.
Also the style of popup menu would be a separate one too. @ilmotta

@qfrank
Copy link
Contributor Author

qfrank commented Nov 9, 2022

Hi @ibrkhalil , thanks for your review, a lot of align task :), can we leave the align task later to make lint-fix? currently, i'm using IntelliJ with cursive plugin, i've made an auto format with IDE, then make lint-fix, but make lint-fix won't align let or map ATM, i hope we can get this pr merged ASAP. @flexsurfer @cammellos thoughts?

I don't know if it helps you, but this Cursive page says aligning let bindings is supported. https://cursive-ide.com/userguide/formatting.html

And you're right, make lint-fix never supported aligning map keys and let bindings, so all of us are using what our IDEs/editors support. If/when we start using zprint, then this tool will actually be able to format map keys and let bindings.

U saved my life! thanks a lot @ilmotta

@ilmotta
Copy link
Contributor

ilmotta commented Nov 9, 2022

I guess above would answer the first difference? On the second difference, i've discussed with @cammellos before that i don't have to implement the bottom part ATM. it would be a separate issue. Also the style of popup menu would be a separate one too. @ilmotta

Thanks for the clarifications @qfrank, it seems we're good for now :)

Changing subject:

Figma shows a menu option with styling, can we do that in the native menu option for Android? It would be great if we could do that, because the acronym BIU without styling feels weird and I'd guess 98% of users don't know what BIU stands for (I didn't tbh).

@qfrank
Copy link
Contributor Author

qfrank commented Nov 9, 2022

I guess above would answer the first difference? On the second difference, i've discussed with @cammellos before that i don't have to implement the bottom part ATM. it would be a separate issue. Also the style of popup menu would be a separate one too. @ilmotta

Thanks for the clarifications @qfrank, it seems we're good for now :)

Changing subject:

Figma shows a menu option with styling, can we do that in the native menu option for Android? It would be great if we could do that, because the acronym BIU without styling feels weird and I'd guess 98% of users don't know what BIU stands for (I didn't tbh).

i believe we can style the popup menu, but it's another issue we need process :)

@ilmotta
Copy link
Contributor

ilmotta commented Nov 9, 2022

i believe we can style the popup menu, but it's another issue we need process :)

Sounds great to me @qfrank. On top of that, in the PR description you mentioned about getting help from someone with iOS experience. I guess that could be another issue as well, right?

@status-im-auto
Copy link
Member

83% of end-end tests have passed

Total executed tests: 6
Failed tests: 1
Passed tests: 5
Not executed tests: 1
IDs of not executed tests: 702777 
IDs of failed tests: 702745 

Not executed tests (1)

Click to expand
  • Rerun not executed tests
  • Failed tests (1)

    Click to expand
  • Rerun failed tests

  • Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745

    Device 2: Looking for a message by text: hello
    Device 2: Find `TimeStampText` by `xpath`: `//*[starts-with(@text,'hello')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']//*[@content-desc='message-timestamp']`

    critical/chats/test_1_1_public_chats.py:1285: in test_1_1_chat_non_latin_messages_stack_update_profile_photo
        timestamp = self.chat_2.chat_element_by_text(messages[0]).timestamp
    ../views/chat_view.py:158: in timestamp
        return TimeStampText(self.driver, self.locator).text
    ../views/base_element.py:204: in text
        return self.find_element().text
    ../views/base_element.py:79: in find_element
        raise NoSuchElementException(
     Device 2: TimeStampText by xpath: `//*[starts-with(@text,'hello')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']//*[@content-desc='message-timestamp']` is not found on the screen
    



    Device sessions

    Passed tests (5)

    Click to expand

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_text_message_delete_push_disappear, id: 702733
    Device sessions

    2. test_1_1_chat_message_reaction, id: 702730
    Device sessions

    Class TestDeeplinkOneDevice:

    1. test_deep_link_open_user_profile, id: 702775
    Device sessions

    2. test_public_chat_open_using_deep_link, id: 702776
    Device sessions

    3. test_deep_link_with_invalid_user_public_key_own_profile_key, id: 702774
    Device sessions

    @churik
    Copy link
    Member

    churik commented Nov 18, 2022

    @qfrank thank you for the update!

    On Xiaomi Mi 9T Pro (Android 10) - there is no option for me at all:
    IMAGE 2022-11-18 14:54:24

    On Samsung Galaxy A52 (Android 11) it can be opened, but not on the first attempt:

    FILE.2022-11-18.14.58.53.mp4

    On Huawei P40 Lite, EMUI 12 (android 11): works as expected.

    I can try some virtual devices if needed, let me know.

    @churik
    Copy link
    Member

    churik commented Nov 18, 2022

    UPD: works fine on Samsung Galaxy with Android10, but not on Xiaomi phones regardless of Android version.

    @qfrank
    Copy link
    Contributor Author

    qfrank commented Nov 18, 2022

    i may need prepare a xiaomi device then :( , i will try it next week, thank you @churik

    @churik
    Copy link
    Member

    churik commented Nov 18, 2022

    we have browserstack! I can share details

    @cammellos
    Copy link
    Contributor

    @churik Do you see a custom menu on whatsapp on Xiaomi phones?

    If no, I'd say this would be a won't fix, otherwise we might look into it.
    Thank you

    @qfrank
    Copy link
    Contributor Author

    qfrank commented Nov 21, 2022

    Hi @churik , this one just get fixed :)

    On Samsung Galaxy A52 (Android 11) it can be opened, but not on the first attempt:

    @VolodLytvynenko VolodLytvynenko self-assigned this Nov 21, 2022
    @VolodLytvynenko
    Copy link
    Contributor

    Hi @qfrank. Thank you for the PR. Please, take a look at the following issues

    ISSUE 1. The text is not shown as styled if a user is typing it within styling frames

    Steps to reproduce:

    1. Open 1-1 chat with any existing contact
    2. Start typing the following: **test

    Actual result:
    Text is not shown in the style
    image

    Expected result:
    The text is shown in style during the user typing it in the style frames
    image

    ISSUE 2. Text is not shown in style if the current text is wrapped into all together styles

    Steps to reproduce:

    1. Open 1-1 chat with any existing contact
    2. Type any text
    3. Select current text and wrap it into a Bold style
    4. Select current text one more time and wrap it in Italic style
    5. Select current text one more time and wrap it in Strikethrough style

    Actual result:
    Text is not shown as styled for all selected styles
    https://user-images.githubusercontent.com/52490791/203312437-3dfc2b6c-e856-40a0-b91d-8de7e614aab2.mp4

    Expected result
    Text is shown as styled for all selected styles

    How it is implemented within WhatsApp
    https://user-images.githubusercontent.com/52490791/203313185-7266f927-0b46-4445-a8c1-4f5dbb3d7c94.mp4

    ISSUE 3. The pop up is not shown with BIU option for the some devices
    ENV:

    1. Device: Xiaomi Redmi Note 11
    2. Device: IPhone 11 pro. Version: 15.6.1

    Actual result:
    For Xiaomi
    image
    For IPhone
    image

    Expected result:
    The selection pop is as per the design and with BIU option
    image

    @churik
    Copy link
    Member

    churik commented Nov 22, 2022

    I guess all the issues can be addressed separetely, not crucial from my POV.
    From this context #14249 (comment) that is implemented only for Android for now.
    @VladimrLitvinenko would you mind checking #14249 (comment) ?
    Thank you!

    @VolodLytvynenko
    Copy link
    Contributor

    @qfrank thank you for your work.
    PR is tested and ready for merge. I have investigated WhatsApp on Xiaomi devices and there is the same behavior
    image

    @qfrank qfrank merged commit 32d85d5 into status-im:develop Nov 23, 2022
    @qfrank
    Copy link
    Contributor Author

    qfrank commented Nov 23, 2022

    notice again, we need the IOS implementation :)

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    No open projects
    Archived in project
    Development

    Successfully merging this pull request may close these issues.

    8 participants