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

list-item suppliment/fixes #8912

Merged
merged 1 commit into from
Sep 11, 2019
Merged

list-item suppliment/fixes #8912

merged 1 commit into from
Sep 11, 2019

Conversation

bitsikka
Copy link
Contributor

@bitsikka bitsikka commented Sep 4, 2019

In kind of a response to this comment by @errorists #8070 (comment)

likely something else that I forgot but that there makes up the majority of our UI.

fixes/adds these:

  • fix title being squished by looong text accessory - it happens in profile>sync settings>mailserver list-item(some of the mailserver names are quite long). I think it is a good idea to limit in general, the length of text-accessory(about 2/3 of device width seems to work best).
    Note: there has always been an option for using component accessory for cases when longer than 2/3 accessory is needed - like transaction signing bottom-pop-up for example
  • have react-key key - useful for homogeneous list-items like chat-list/asset-list/list-with-radio-buttons etc. Default key generated by handy gensym CLJS core utility.
  • add :selectable :theme together with :selected? config key for list-items with radio accessory
  • bring list-item/divider within list-item by having a type called divider
    • prevents having to require list-item namespace just for that
    • keeps data fed to flat-list with list-items as clean as possible
  • docs in code updated accordingly
  • screens with instances of list-items updated accordingly

Status: ready

@bitsikka bitsikka requested a review from a team as a code owner September 4, 2019 15:46
@auto-assign auto-assign bot removed the request for review from a team September 4, 2019 15:46
@ghost
Copy link

ghost commented Sep 4, 2019

Pull Request Checklist

  • Have you updated the documentation, if impacted (e.g. docs.status.im)?

@status-im-auto
Copy link
Member

status-im-auto commented Sep 4, 2019

Jenkins Builds

Click to see older builds (36)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 22037ef #1 2019-09-04 15:58:48 ~12 min ios 📦 ipa
✔️ 22037ef #1 2019-09-04 15:59:14 ~12 min android 📦 apk
22037ef #1 2019-09-04 16:02:30 ~16 min android-e2e 📄 log
✔️ 2b96205 #2 2019-09-04 18:38:09 ~11 min android 📦 apk
✔️ 2b96205 #2 2019-09-04 18:38:15 ~11 min android-e2e 📦 apk
✔️ 2b96205 #2 2019-09-04 18:38:35 ~11 min ios 📦 ipa
397dbfa #3 2019-09-05 11:47:01 ~11 min ios 📄 log
397dbfa #3 2019-09-05 11:50:56 ~15 min android 📄 log
397dbfa #3 2019-09-05 11:51:02 ~15 min android-e2e 📄 log
565ebbf #4 2019-09-05 13:19:05 ~10 min ios 📄 log
565ebbf #4 2019-09-05 13:22:38 ~14 min android-e2e 📄 log
565ebbf #4 2019-09-05 13:23:35 ~15 min android 📄 log
be3e1b4 #5 2019-09-05 16:14:41 ~13 min android 📄 log
be3e1b4 #5 2019-09-05 16:14:46 ~13 min android-e2e 📄 log
be3e1b4 #5 2019-09-05 16:15:35 ~14 min ios 📄 log
db27fb5 #6 2019-09-05 16:52:26 ~11 min ios 📄 log
db27fb5 #6 2019-09-05 16:55:44 ~14 min android-e2e 📄 log
db27fb5 #6 2019-09-05 16:56:27 ~15 min android 📄 log
bec874d #7 2019-09-06 02:43:58 ~10 min android-e2e 📄 log
bec874d #7 2019-09-06 02:44:34 ~11 min android 📄 log
bec874d #7 2019-09-06 02:44:34 ~11 min ios 📄 log
bbcdeda #8 2019-09-06 07:58:21 ~11 min ios 📄 log
bbcdeda #8 2019-09-06 08:01:56 ~14 min android-e2e 📄 log
bbcdeda #8 2019-09-06 08:03:42 ~16 min android 📄 log
✔️ f0bcdd5 #9 2019-09-06 08:49:16 ~11 min ios 📦 ipa
✔️ f0bcdd5 #9 2019-09-06 08:56:14 ~18 min android 📦 apk
✔️ f0bcdd5 #9 2019-09-06 08:56:28 ~18 min android-e2e 📦 apk
✔️ 54620d4 #10 2019-09-06 14:15:14 ~10 min ios 📦 ipa
✔️ 54620d4 #10 2019-09-06 14:19:36 ~15 min android-e2e 📦 apk
✔️ 54620d4 #10 2019-09-06 14:20:31 ~16 min android 📦 apk
✔️ 1a683a3 #11 2019-09-09 13:46:02 ~10 min ios 📦 ipa
✔️ 1a683a3 #11 2019-09-09 13:46:20 ~11 min android-e2e 📦 apk
✔️ 1a683a3 #11 2019-09-09 13:47:37 ~12 min android 📦 apk
✔️ 38bca45 #12 2019-09-09 15:41:22 ~10 min android-e2e 📦 apk
✔️ 38bca45 #12 2019-09-09 15:41:36 ~10 min android 📦 apk
✔️ 38bca45 #12 2019-09-09 15:42:11 ~11 min ios 📦 ipa
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ f94f7b3 #14 2019-09-09 17:35:51 ~9 min ios 📦 ipa
✔️ f94f7b3 #14 2019-09-09 17:37:21 ~11 min android-e2e 📦 apk
✔️ f94f7b3 #14 2019-09-09 17:45:20 ~19 min android 📦 apk
✔️ 7fd56ed #15 2019-09-09 17:56:06 ~8 min ios 📦 ipa
✔️ 7fd56ed #15 2019-09-09 17:59:37 ~11 min android 📦 apk
✔️ 7fd56ed #15 2019-09-09 18:00:28 ~12 min android-e2e 📦 apk

@bitsikka
Copy link
Contributor Author

bitsikka commented Sep 5, 2019

  • fixed actionably clear outstanding issues
  • rebased

@flexsurfer
Copy link
Member

flexsurfer commented Sep 6, 2019

@bitsikka could you resolve conflicts please, i've made some fixes and changes in list-item, removed blue theme and fixed container styles, please tell if you need any help with that, thanks

@bitsikka
Copy link
Contributor Author

bitsikka commented Sep 6, 2019

@bitsikka could you resolve conflicts please, i've made some fixes and changes in list-item, removed blue theme and fixed container styles, please tell if you need any help with that, thanks

@flexsurfer
I've resolved it locally, but I can't seem to launch dev build(iOS) to check for correctness. It builds fine even launches in emulator but gets stuck on Status logo screen. So I have not been able to progress on this or other issue I have committed to work on.

Also I noted that :blue theme related stuff is not completely removed, which I would clean-up if I could get dev build running on emulator.

So, I'm kind of waiting for flurry of changes in the pipeline 👍 🚀 , hoping that will fix the build problem 😄

@bitsikka
Copy link
Contributor Author

bitsikka commented Sep 6, 2019

I'm pushing it anyway.. without clean-up to remeaining :blue theme stuff
consider it a wip
I think I can cleanup more about the :background-color in styles/container as well

@flexsurfer
Copy link
Member

@bitsikka is it ready for testing?

@bitsikka
Copy link
Contributor Author

bitsikka commented Sep 9, 2019

@bitsikka is it ready for testing?

@flexsurfer
Not yet
having this issue #8447 (comment)

@flexsurfer
Copy link
Member

@bitsikka please rebase, should be fixed now

@bitsikka
Copy link
Contributor Author

bitsikka commented Sep 9, 2019

@bitsikka please rebase, should be fixed now

👍 Thanks! life saver :)
I will wrap this up first soon
and move on to the design touchup wallet ui stuff

@bitsikka
Copy link
Contributor Author

bitsikka commented Sep 9, 2019

@flexsurfer

  • resolved conflict
  • brought back :min-height in styles/container and gave reasons in the comment.
  • cleaned up :blue theme handelings hanging around
  • and other tiny cleanups

One more thing I need a decision on:

I can think of 1 instance where :main-icons/share is being used - in other people's profile

this should keep the :accessories vector as clean as before, but more flexible.

@errorists
Copy link
Contributor

errorists commented Sep 9, 2019

for icon accessory can we just have list-item accept just :main-icons/icon and remove :check and :more option?

Yes we can! in design it's easy enough to override and on occasions we did just that like you pointed out in profiles. I imagine if possible, it would be nice to default to chevron as it's the vast majority of our use cases.

if we argree on all :main-icons/icon accessory to be gray colored we can just keep the 3rd case and remove :check and :more cc @errorists

I think we can agree on all of them being 40% dark grey, yes. Regular dark grey is too much for something as secondary.

edit: so to sum up, in the component you can customise which icon is used in the right side accessory, the appearance is unified at 40% dark grey like we use for chevrons currently.

@bitsikka
Copy link
Contributor Author

bitsikka commented Sep 9, 2019

for icon accessory can we just have list-item accept just :main-icons/icon and remove :check and :more option?

Yes we can! in design it's easy enough to override and on occasions we did just that like you pointed out in profiles. I imagine if possible, it would be nice to default to chevron as it's the vast majority of our use cases.

if we argree on all :main-icons/icon accessory to be gray colored we can just keep the 3rd case and remove :check and :more cc @errorists

I think we can agree on all of them being 40% dark grey, yes. Regular dark grey is too much for something as secondary.

Cool! this will help keep list-item implementation towards lean

yes :chevron unlike other (24x24)icons is 10x16 and is such a special case, that I did not mention and kept :)

@bitsikka
Copy link
Contributor Author

bitsikka commented Sep 9, 2019

I'm going to do this #8955 (comment) for Mailserver like loong content requiring :type :small list-item instances, and it should be ready

@bitsikka
Copy link
Contributor Author

bitsikka commented Sep 9, 2019

I'm going to do this #8955 (comment) for Mailserver like loong content requiring :type :small list-item instances, and it should be ready

Did this (works pretty good - in line with updated spec):

[list-item/list-item
     {:type                :small
      :accessibility-label :node-version
      :title-prefix        :t/node-version
      :title
      [react/text
       {:number-of-lines 1
        :ellipsize-mode  :middle
        :style
        {:color        colors/gray
         :padding-left 16
         :text-align   :right
         :line-height  22}}
       node-version]}]

Kept 62% of device width limit on Accessory text - can still hack/by-pass-limit with component as accessory or like above

For list-items with long accessories, better to hack list-item like this than put it in accessories vector because of the implementation rationale described here #8955 (comment).

fin :)

@churik
Copy link
Member

churik commented Sep 10, 2019

@flexsurfer @yenda please let me know when this one will be ready for testing

@flexsurfer
Copy link
Member

@bitsikka is it ready?

@bitsikka
Copy link
Contributor Author

@bitsikka is it ready?
@flexsurfer @churik
Yes, it is ready

@statustestbot
Copy link

93% of end-end tests have passed

Total executed tests: 46
Failed tests: 3
Passed tests: 43

Failed tests (3)

Click to expand
1. test_deploy_contract_from_daap

Device 1: Tap on OkButton
Device 1: Looking for an element by text: 'Contract deployed at: '

Contract was not created

Device sessions

2. test_ens_in_public_chat

Device 1: Tap on EnsCheckName
Device 1: Looking for an element by text: 'Ok, got it'

Device 1: 'BaseButton' is not found on the screen

Device sessions

3. test_long_press_to_delete_public_chat

Device 1: Looking for a message by text: 'test message'
Device 1: Wait for ChatElementByText

Chat history is shown

Device sessions

Passed tests (43)

Click to expand
1. test_block_user_from_public_chat
Device sessions

2. test_filters_from_daap
Device sessions

3. test_copy_and_paste_messages
Device sessions

4. test_send_transaction_from_daap
Device sessions

5. test_open_transaction_on_etherscan
Device sessions

6. test_public_chat_messaging
Device sessions

7. test_long_press_to_delete_1_1_chat
Device sessions

8. test_password_in_logcat_sign_in
Device sessions

9. test_text_message_1_1_chat
Device sessions

10. test_add_to_contacts
Device sessions

11. test_sign_typed_message
Device sessions

12. test_unread_messages_counter_1_1_chat
Device sessions

13. test_logcat_send_transaction_from_daap
Device sessions

14. test_send_message_in_group_chat
Device sessions

15. test_logcat_send_transaction_from_wallet
Device sessions

16. test_send_token_with_7_decimals
Device sessions

17. test_offline_messaging_1_1_chat
Device sessions

18. test_modify_transaction_fee_values
Device sessions

19. test_send_eth_from_wallet_to_address
Device sessions

20. test_add_account_to_multiaccount_instance
Device sessions

21. test_manage_assets
Device sessions

22. test_send_emoji
Device sessions

23. test_search_chat_on_home
Device sessions

24. test_logcat_recovering_account
Device sessions

25. test_can_add_existing_ens
Device sessions

26. test_messaging_in_different_networks
Device sessions

27. test_logcat_sign_message_from_daap
Device sessions

28. test_switch_users_and_add_new_account
Device sessions

29. test_send_stt_from_wallet
Device sessions

30. test_login_with_new_account
Device sessions

31. test_start_chat_with_ens
Device sessions

32. test_add_contact_from_public_chat
Device sessions

33. test_send_two_transactions_one_after_another_in_dapp
Device sessions

34. test_password_in_logcat_creating_account
Device sessions

35. test_backup_recovery_phrase
Device sessions

36. test_offline_status
Device sessions

37. test_open_google_com_via_open_dapp
Device sessions

38. test_unread_messages_counter_public_chat
Device sessions

39. test_sign_message_from_daap
Device sessions

40. test_user_can_remove_profile_picture
Device sessions

41. test_share_contact_code_and_wallet_address
Device sessions

42. test_refresh_button_browsing_app_webview
Device sessions

43. test_backup_recovery_phrase_warning_from_wallet
Device sessions

@bitsikka
Copy link
Contributor Author

Sorry, didn't post any testing notes

Testing notes

  • The supplementary features mentioned in OP above, are not manifested anywhere visually yet - no need for testing
  • Test Mailserver title is not squished by looong mailserver name
  • Test Node version title is not squished by looong node alpha-numeric version

@statustestbot
Copy link

100% of end-end tests have passed

Total executed tests: 1
Failed tests: 0
Passed tests: 1

Passed tests (1)

Click to expand
1. test_ens_in_public_chat
Device sessions

@churik
Copy link
Member

churik commented Sep 11, 2019

awesome work!
Thank you @bitsikka !

Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>
@flexsurfer flexsurfer merged commit f981dbb into status-im:develop Sep 11, 2019
@bitsikka bitsikka deleted the list-item-suppliment/fixes branch September 11, 2019 15:31
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.

9 participants