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

Fix oversize label in no result side menu on iPad #19305

Merged
merged 6 commits into from
Sep 30, 2022
Merged

Fix oversize label in no result side menu on iPad #19305

merged 6 commits into from
Sep 30, 2022

Conversation

ipalm0423
Copy link
Contributor

@ipalm0423 ipalm0423 commented Sep 13, 2022 •
edited
Loading

Fixes #19244 #19246

Description:

This PR fixes the issue where the labels are oversizing in its container view.

• Before After
iPad Q1 Q1-fix
iPad-RTL Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-09-19 at 01 17 04 Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-09-19 at 01 12 12

Root Cause:

In the xib NoResults.storyboard, the container view of the labels only has the width constraint Width <= 360, but it doesn't have a constraint related to its super view. This will let the labels go outside if its super view is smaller than the label's content.
Constraint on superView
In this case, the side menu view only has width == 320. The container view will still go up to 360 because it doesn't care about its super view size.

Fix:

  • Add missing width constraint between the container view and its super view

Test Step:

  1. Launch the app.
  2. Log in with an account without any notification or sign up for a new account.
  3. Navigate to the Notifications or My Site tab.
  4. Notice the text should fit inside the left layout.

Regression Notes

  1. Potential unintended areas of impact
  • I list all impacted pages in here with screenshots.
  1. What I did to test those areas of impact (or what existing automated tests I relied on)
  • Manually test on all impacted pages on iPad, and key pages on iPhone, and list the test result here
  1. What automated tests I added (or what prevented me from doing so)
    add unitTest at 41c4cb3 for testing labels width in different sizes of the container.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • 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.

@twstokes twstokes self-requested a review September 14, 2022 15:45
@twstokes
Copy link
Contributor

Any views that present NoResultViewController

👋 @ipalm0423 - do you think we should list out the views that use NoResultsViewController so when potentially non-technical testers test these changes, they'll know some to try? Thanks!

@ipalm0423
Copy link
Contributor Author

ipalm0423 commented Sep 18, 2022 •
edited
Loading

Any views that present NoResultViewController

👋 @ipalm0423 - do you think we should list out the views that use NoResultsViewController so when potentially non-technical testers test these changes, they'll know some to try? Thanks!

Sure, I created a view list that is inspected on this PR.

Also, I added some tests in the 41c4cb3. To test the NoResultsViewController label width in different width of container.

@ipalm0423 ipalm0423 force-pushed the fix/19244-19246-fix-oversize-label-on-NoResultVC-for-iPad branch from 0d8226d to 41c4cb3 Compare September 21, 2022 15:41
@ipalm0423 ipalm0423 marked this pull request as ready for review September 21, 2022 16:01
@twstokes twstokes mentioned this pull request Sep 21, 2022
Merged
@twstokes
Copy link
Contributor

CI branch: #19333

@twstokes twstokes added this to the 20.9 milestone Sep 23, 2022
twstokes
twstokes reviewed Sep 23, 2022
View reviewed changes
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

This looks really good @ipalm0423! Thanks for being so thorough with your manual tests and screenshots. 🙇 The unit tests are great, too.

I noticed the margins on this view were a little tight. Do you think we should tweak this?

@ipalm0423
Copy link
Contributor Author

ipalm0423 commented Sep 24, 2022 •
edited
Loading

This looks really good @ipalm0423! Thanks for being so thorough with your manual tests and screenshots. 🙇 The unit tests are great, too.

I noticed the margins on this view were a little tight. Do you think we should tweak this?

Yes, we could tweak the horizontal margin to make it look nice.
Do we have the UI design specification for this margin size? (Figma, Zeplin...etc?)

I only found this spec, but not sure if it's the correct one.

@twstokes
Copy link
Contributor

Yes, we could tweak the horizontal margin to make it look nice. Do we have the UI design specification for this margin size? (Figma, Zeplin...etc?)

I only found this spec, but not sure if it's the correct one.

Typically we would pull in a designer for Figma designs, but how do you feel about adding just a few points of margin on the sides so it's not as snug?

@ipalm0423
Copy link
Contributor Author

Typically we would pull in a designer for Figma designs, but how do you feel about adding just a few points of margin on the sides so it's not as snug?

I tweaked the margin for 20 on f05670e , also modify the unit-test

Tested on the narrow key pages:

margin 20 default RTL
iPhone iphone13- Simulator Screen Shot - iPhone 13 - 2022-09-28 at 16 52 54
iPad iPad9 ipad9-rtl

@twstokes twstokes self-requested a review September 28, 2022 16:53
@twstokes
Copy link
Contributor

I tweaked the margin for 20 on f05670e , also modify the unit-test

Great! Thanks for doing that. I've pushed the latest to the CI branch to run tests. 👍

twstokes
twstokes reviewed Sep 29, 2022
View reviewed changes
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

@ipalm0423 can we update RELEASE-NOTES.txt for these changes? Thanks!

@ipalm0423 ipalm0423 force-pushed the fix/19244-19246-fix-oversize-label-on-NoResultVC-for-iPad branch from f05670e to ec96ae6 Compare September 29, 2022 09:51
@peril-wordpress-mobile
Copy link

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

Generated by 🚫 dangerJS

@ipalm0423
Copy link
Contributor Author

@ipalm0423 can we update RELEASE-NOTES.txt for these changes? Thanks!

No, problem. Rebased to trunk, and update the release note at ec96ae6.

@twstokes
Copy link
Contributor

Latest has been pushed to CI branch.

@twstokes twstokes self-requested a review September 30, 2022 00:45
twstokes
twstokes approved these changes Sep 30, 2022
View reviewed changes
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

LGTM @ipalm0423!

@twstokes twstokes merged commit ef7dad3 into wordpress-mobile:trunk Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twstokes twstokes twstokes approved these changes

Assignees

@ipalm0423 ipalm0423

Labels
[Type] Bug
Projects
None yet
Milestone
20.9
Development

Successfully merging this pull request may close these issues.

Layout problem on empty My Site
2 participants
@ipalm0423 @twstokes