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

Show currently selected site in site visibility editor screen #13160

Merged
merged 12 commits into from
Dec 24, 2024
Merged

Show currently selected site in site visibility editor screen #13160

merged 12 commits into from
Dec 24, 2024

Conversation

JorgeMucientes
Copy link
Contributor

@JorgeMucientes JorgeMucientes commented Dec 18, 2024 •
edited by irfano
Loading

Closes: #13159

Description

Adds a new section to the "Edit site visibility" screen displaying the currently selected site. Additionally, it filters out the currently selected site from the list.

Testing information

  1. Log in with an account that has multiple sites
  2. Click Edit Stores button
  3. Check the currently selected site is displayed on the header
  4. Check the currently selected site is not displayed in the editable list of sites

The tests that have been performed

The above

Images/gif

  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 18, 2024 •
edited
Loading

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit23e077c
Direct Downloadwoocommerce-wear-prototype-build-pr13160-23e077c.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 18, 2024 •
edited
Loading

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit23e077c
Direct Downloadwoocommerce-prototype-build-pr13160-23e077c.apk

@JorgeMucientes JorgeMucientes changed the base branch from trunk to issue/13136-manage-stores-visibility December 18, 2024 15:26
@JorgeMucientes JorgeMucientes added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Dec 18, 2024
@JorgeMucientes JorgeMucientes marked this pull request as ready for review December 18, 2024 15:26
@JorgeMucientes JorgeMucientes mentioned this pull request Dec 20, 2024
Merged
4 tasks
Base automatically changed from issue/13136-manage-stores-visibility to trunk December 20, 2024 09:03
@JorgeMucientes JorgeMucientes added this to the 21.4 milestone Dec 20, 2024
@JorgeMucientes JorgeMucientes added feature: site picker Any task related to site picker screen and removed status: do not merge Dependent on another PR, ready for review but not ready for merge. labels Dec 20, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 40.46%. Comparing base (de496ec) to head (23e077c).
Report is 420 commits behind head on trunk.

Files with missing lines Patch % Lines
...cker/sitevisibility/WooSitesVisibilityViewModel.kt 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##              trunk   #13160      +/-   ##
============================================
- Coverage     40.47%   40.46%   -0.01%     
  Complexity     6237     6237              
============================================
  Files          1303     1303              
  Lines         75509    75512       +3     
  Branches      10361    10362       +1     
============================================
- Hits          30560    30559       -1     
- Misses        42288    42290       +2     
- Partials       2661     2663       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@irfano irfano self-assigned this Dec 21, 2024
irfano
irfano requested changes Dec 21, 2024
View reviewed changes
Copy link
Contributor

@irfano irfano left a comment

Choose a reason for hiding this comment

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

The design and code look good. However, I noticed some issues:

  • When the list is too long, the footer is not visible.
  • Perhaps we should consider making the entire screen scrollable instead of just the “Other Sites” section, as the current experience in landscape orientation is not ideal.

...tlin/com/woocommerce/android/ui/sitepicker/sitevisibility/WooSitesVisibilityViewModelTest.kt Show resolved Hide resolved
@JorgeMucientes
Copy link
Contributor Author

Thanks for the review @irfano! Good points!

I fixed both of them. To do that I made the whole screen scrollable and added the footer as subtitle so it is always visible:

demoFixes.mp4

The alternative approach was to anchor the footer to the bottom, but since it is a very small text it didn't look well. I did a quick test (without adding the paddings or enything) and wasn't convinced about it. Let me know what you think.

footerAnchored.mp4

@irfano
Copy link
Contributor

irfano commented Dec 24, 2024

I don’t have a strong opinion, but placing the text at the top looks good. cc @itsmeichigo

@irfano irfano merged commit e8a661f into trunk Dec 24, 2024
16 checks passed
@irfano irfano deleted the issue/13107-show-current-site-header branch December 24, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@irfano irfano irfano approved these changes

@hafizrahman hafizrahman Awaiting requested review from hafizrahman

Assignees

@irfano irfano

Labels
feature: site picker Any task related to site picker screen
Projects
None yet
Milestone
21.4
Development

Successfully merging this pull request may close these issues.

Show currently selected site as header of available stores to hide list
4 participants
@JorgeMucientes @wpmobilebot @codecov-commenter @irfano