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

Move me to bottom nav #18606

Merged
merged 22 commits into from
Jun 22, 2023
Merged

Move me to bottom nav #18606

merged 22 commits into from
Jun 22, 2023

Conversation

ravishanker
Copy link
Contributor

@ravishanker ravishanker commented Jun 15, 2023

Moves Me (avatar) to Bottom Navigation as a fourth tab after Notifications on JP app, and in menu under manage on WP app

  • Updates MySite Info layout
  • Updates Me screen layout

Fixes # pcdRpT-2HW-p2#comment-4875, and this discussion p1686812471360099-slack-C04SFL0RP51

Jetpack app

Me at the Bottom Me Screen Dark Mode
Screenshot_20230618_160803 Screenshot_20230618_160742 Screenshot_20230618_162015

WordPress app

Me in menu Me
Screenshot_20230620_171528 Screenshot_20230620_171538

To test:

  • Launch Jetpack app, and login

  • Verify that Me/Avatar doesn't appear at top right hand corner anymore

  • Verify that Me is in the bottom navigation, next to Notifications

  • Tap on Me tab

  • Verify that it goes to Me Screen as before

  • Launch WordPress app, and login

  • Verify that Me/Avatar doesn't appear at top right hand corner anymore

  • Verify that Me is in the menu, under manage category

  • Tap on Me row

  • Verify that it goes to Me Screen as before

To-do for later in a separate PR

  • Replace icons with new ones on Me Screen
  • Add change photo cta on Me -> My Profile screen if required

Regression Notes

  1. Potential unintended areas of impact
    N/A

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A

  3. What automated tests I added (or what prevented me from doing so)
    Existing tests

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 15, 2023

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr18606-7b5b2ab
Commit7b5b2ab
Direct Downloadwordpress-prototype-build-pr18606-7b5b2ab.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 15, 2023

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr18606-7b5b2ab
Commit7b5b2ab
Direct Downloadjetpack-prototype-build-pr18606-7b5b2ab.apk
Note: Google Login is not supported on these builds.

Copy link

@osullivanchris osullivanchris left a comment

Choose a reason for hiding this comment

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

Sharing per slack the reasons I think this is a good move:

  1. the iA is more 'correct'. Today, 'Me' is inside My Site, which is the opposite of the actual parent/child structure
  2. its more consistent with WordPress.com where the top bar has My Sites, Reader, Me, Notifications
  3. When we merged Me into My Site years ago, My Site was just a menu. Now My Site is more complicated and we have several rows of navigations/information at the top
  4. We're using a lot of vertical space for only one icon
  5. Removing that vertical space aligns the header information on My Site, Reader, Notifications more consistently

Here are a couple of improvements I think we could make. Wouldn't have to do all immediately and some are separate issues. The padding bug is a must fix of course. Then, I think changing the layout of the profile photo and name on 'Me' to match the My Site header would be a big improvement, if I was to pick one change.

Me-Tab-Tweaks

Moved Me avatar to Bottom Nav
- Reduce blavatar size from 64 to 40
- Reduce chevron size from 24 to 16
- Update to new header layout similar to My Site
- Removed CHANGE PHOTO cta
- Updated paddings and margins
- clean up
Adjust app_bar_with_site_info_tabs_height from 200 dp to 136dp since Me is removed now
- increase the Site icon on My Site to 56dp
- Increase Me icon size to 56dp, and align row icons to Center of Me icon on Me screen
@ravishanker ravishanker requested review from ovitrif and pachlava June 19, 2023 00:17
@ravishanker ravishanker marked this pull request as ready for review June 19, 2023 00:21
@ravishanker
Copy link
Contributor Author

👋 @ovitrif - I've added you as a reviewer since you're also doing some work in this area. 🙇

@ravishanker
Copy link
Contributor Author

👋 @pachlava - I've added you as this might fail instrumented tests, any other automation stuff. 🙇

@AjeshRPai
Copy link
Contributor

AjeshRPai commented Jun 19, 2023

Hey @ravishanker 👋🏼

Awesome job on moving me to the bottom nav 🙌🏼 . Sorry for jumping in, but I was doing some changes in the dashboard and noticed this PR.

FYI - On jetpack phase removal four, new users phase and self hosted users phase, we are removing this bottom nav entirely to prevent the access of reader and notifications. you might need to make some changes to show the bottom nav without a reader and notifications but only with me page and my site bottom nav in the above phases.

⚠️ 🐛 I tested this PR and enabled the jp_removal_four flag, and the bottom nav is completely gone. So a user can't access the me page at all. Screenshot below.

I also noticed the padding issue, but I am unsure if that's a WIP. If thats fixed, let me know so I can double-check.

also, There is an issue int the enabling of phase 4 right now which we are trying to fix with #18612,

TO reproduce the issue of the me tab not shown, you can try any of the below scenarios

  1. Disable jp_removal_static_posters_phase and enable jp_removal_four
  2. IF you are testing this scenario after this PR is merged, then just enable jp_removal_four
  3. enable jp_removal_new_users - this phase is rolled out, in this phase notifications and reader is not shown
  4. enable jp_removal_self_hosted_users- this phase is rolled out as well, in this phase notifications and reader is not shown as well

Let me know if you need any help with the changes that need to be done or want more information on this.

@osullivanchris
Copy link

@AjeshRPai thanks for taking the time to look and highlighting the changes on the WordPress side. That wasn't front in my mind. I'm going to take a look at it. But I think the 'default' solution would be having a tab bar with 2 tabs as you described.

Another approach would be to keep the old layout with 'me' in the My Site navigation bar only for WordPress, but that seems like more differences between the two apps to support and maintain than a tab bar with items which turn on and off...

@ravishanker
Copy link
Contributor Author

ravishanker commented Jun 20, 2023

Let me know if you need any help with the changes that need to be done or want more information on this.

@AjeshRPai - Thanks for noticing, and flagging this early 🙇 I'll need to make some changes then in the light phase four. I'll add you as a reviewer as well to make sure it all works before and after.

This can be done in two ways

  • Bottom nav with My Site, and Me
  • Me under Manage in the menu (ignore profile pic, and padding for now)
Bottom Nav Manage
Screenshot_20230620_142832 Screenshot_20230620_141159

@ravishanker ravishanker requested a review from AjeshRPai June 20, 2023 04:25
@ovitrif
Copy link
Contributor

ovitrif commented Jun 20, 2023

👋 @ovitrif - I've added you as a reviewer since you're also doing some work in this area. 🙇

This is a no-brainer for me, I like it, though as @AjeshRPai shared, there is some overlap with ongoing work to improve the daily development workflow, by optimising and making debug settings easily accessible regardless of logged in/out state.

Then there's also the phase 4, which Chris is aware of.

Thus carry on from my side 🙏🏻 , I'm not needed here since I ended up focusing on crashes and didn't do any Compose refactoring 😅 .

@ravishanker ravishanker removed the request for review from ovitrif June 20, 2023 22:43
Copy link
Contributor

@AjeshRPai AjeshRPai left a comment

Choose a reason for hiding this comment

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

Hey @ravishanker

I tested the following scenarios

  • migration from static posters phase to phase 4| self hosted users phase | new users phase 🟢
    • Wordpress app
      • Bottom nav is not shown on phase 4 and me item is shown in menu list 🟢
      • if the user is on me fragment on static posters phase and the app goes to background, when the phase 4 is switched on user is navigated to my site dashboard without bottom nav. 🟢
    • Jetpack app
      • no ui changes when user migrates 🟢
  • if me tab was the last opened tab, when a user opens the app again, me tab is shown 🟢

Minor issues

  • In wordpress app (static posters phase) me option is show on the menu items as well on bottom nav - as per my understanding it should be shown either on menu items or on bottom nav, correct me if am wrong
  • in both wp/jp app people icon is a dark circle/white circle
light mode dark mode
screenshot_my_site_wordpress__ screenshot_my_site_wordpress_darkmode__

Other than these minor issues, all looks good to me 👍🏼

cc: @zwarm

  1. for letting you about this change on dashboard, in case if you want to test for any regression issues
  2. also for letting you know that I have tested to make sure that there are no regression issues that we fixed for phase 4. mainly 👇🏼
    1. Jetpack Focus: Fix the Landing page content being shown multiple times on top of each other #17811
    2. Jetpack Focus: Fix landing page content scrolling issue #17828

} else {
mFabTooltip.hide();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

👏🏼 thanks for removing this tooltip, ICYMI you can close this issue once this PR is merged
cc: @thomashorta

@tiagomar
Copy link
Contributor

Hey @ravishanker, it seems like we could save some vertical space in Landscape mode.

image

@tiagomar
Copy link
Contributor

🐛 When selecting Me tab, TalkBack reads the My Site description.

meTab.mp4

Fix Me item showing on WP in both menu and on bottom nav during static posters phase
@ravishanker
Copy link
Contributor Author

Minor issues

  • In wordpress app (static posters phase) me option is show on the menu items as well on bottom nav - as per my understanding it should be shown either on menu items or on bottom nav, correct me if am wrong
  • in both wp/jp app people icon is a dark circle/white circle

👋 @AjeshRPai - Thanks for testing for various phases. Fixed the those minor issues. Please review again. 🙇

@ravishanker
Copy link
Contributor Author

Hey @ravishanker, it seems like we could save some vertical space in Landscape mode.

👋 @tiagomar - This is happening on JP app only in landscape mode right? It is proving difficult fix this due to various feature flags and differences between WP/JP etc. Need to handle separately perhaps afterwards

@ravishanker
Copy link
Contributor Author

🐛 When selecting Me tab, TalkBack reads the My Site description.

meTab.mp4

Me tab is reading the content description correctly. When on Me screen, it is reading the MySite description. Still investigating where that is coming from!

@ravishanker ravishanker requested a review from AjeshRPai June 22, 2023 02:13
@ravishanker
Copy link
Contributor Author

👋 @pachlava - If you could please take a look at the failing instrumentation tests, if you're back from AFK . 🙇

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 22, 2023

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@pachlava
Copy link
Contributor

I'm taking a look @ravishanker 👋

@pachlava
Copy link
Contributor

@ravishanker 👋

A 0db1912 of yours updates the flow to get to the Me screen 👍, however, there remained one thing that also needed an update: a way to go back from Me screen.

Previously, this was done by pressing the back button, since the navbar was not available for Me screen, and it resulted in My site screen being shown. Now, however, pressing back results in the app being minimized. So the back button press had to be replaced with tapping My Sites, which I did in 744b3fe. All the tests passed locally for me, 🤞 for the 🟢 Buildkite.

Apparently, the only place where going back from Me screen was needed, was after running e2eLoginWithSelfHostedAccount. It can be seen from the logs that all the UI tests were failing after this test - since to "log out" from the self-hosted account, we need to open Me screen, see that it's a self-hosted account, and then go back to My sites and delete a site.

@tiagomar
Copy link
Contributor

tiagomar commented Jun 22, 2023

In WordPress app, as long as Me is a tab, we probably want to remove the back arrow on top left corner? It will be needed when Me is moved to Menu though.

Co-authored-by: Ravi <ravi.program@gmail.com>

fixes the back button shown on me fragment in wordpress app in static
 posters phase
# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/main/MeFragment.kt
@AjeshRPai
Copy link
Contributor

Hey @ravishanker

The people icon and the me option in menu is fixed, but I found the same bug as @tiagomar reported here. the back button is visible on wordpress app in static posters phase. In an effort to merge this change before the code freeze on monday and also because your are AFK, I went ahead and fixed it with the commit 60fb503

@zwarm am adding you as a reviewer, once you have reviewed the changes i have done you can go ahead and merge it. 👍🏼

I have tested the jetpack app and wordpress app for all other scenarios, everything looks good to me. 🟢

@tiagomar if you can review it once again, it would be awesome 🙌🏼

@AjeshRPai AjeshRPai requested a review from zwarm June 22, 2023 13:27
Copy link
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

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

@AjeshRPai - I've verified the following

  • The back button does not show in JP/WP when "me" is a tab
  • The back button shows in WP when "me" is in the menu
    👍

@ravishanker @osullivanchris - love this change. 🙇‍♀️

@zwarm zwarm merged commit a86b655 into trunk Jun 22, 2023
@zwarm zwarm deleted the Move-Me-to-Bottom-Nav branch June 22, 2023 16:09
@tiagomar
Copy link
Contributor

@tiagomar if you can review it once again, it would be awesome 🙌🏼

It was all good, @AjeshRPai! 👍

Me tab is reading the content description correctly. When on Me screen, it is reading the MySite description. Still investigating where that is coming from!

Yep, I've filed a ticket (#18698 ) for better tracking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants