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

Notification Center: iPad adjustments #4152

Merged
merged 8 commits into from Apr 15, 2022
Merged

Notification Center: iPad adjustments #4152

merged 8 commits into from Apr 15, 2022

Conversation

mcleinman
Copy link
Contributor

@mcleinman mcleinman commented Mar 11, 2022

Phabricator: https://phabricator.wikimedia.org/T286607

Notes

  • In phab ticket, asked for approval on margins being a bit different than proposed - using readableContentGuide instead of requested exact numbers. Olga and Carolyn are comfortable with this.
  • iPad cells now smaller. For larger font sizes, cell size will grow.
  • Changed the sourceRect for the popover so it doesn't block the content. Current behavior was impact of giving a source size of zero.
  • As discussed in meeting, we don't have control over the font size or truncation style of pop overs or action sheets, the fourth part of this ticket. If we want to control this, we'd need to switch to another type of control.

Test Steps

  1. Run app, look at Notifications Center and its submenus.
  2. Check on both iPad and iPhone, and in various colors

Screenshots/Videos

Filter margins (before/after)
Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-03-11 at 13 17 31 Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-03-11 at 13 28 41

Inbox margins (before/after)
Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-03-11 at 13 30 18 Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-03-11 at 14 02 33

Updated anchor for popover menu
Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-03-11 at 14 33 46

Main screen - margins and cell height (before/after)
Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-03-11 at 14 23 33 Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-03-11 at 14 23 36

Large fonts
Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-03-11 at 14 35 45
![Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-03-11 at 14 35 54](https://user-images.githubusercontent.com/9295855/157983868-9d10584d-0c35-4904-b961-
Simulator Screen Shot - iPad Pro (9 7-inch) - 2022-03-11 at 14 36 16
be122cac94ec.png)

@mcleinman mcleinman added the Design Question(s) Tag @olga-ti label Mar 11, 2022
@mcleinman mcleinman requested review from carolynlimadeo and a team and removed request for carolynlimadeo March 11, 2022 22:43
@mcleinman mcleinman removed the Design Question(s) Tag @olga-ti label Mar 16, 2022
@mcleinman mcleinman requested review from mazevedofs and removed request for a team March 22, 2022 18:36
@tonisevener tonisevener requested review from tonisevener and removed request for mazevedofs March 31, 2022 16:11
@tonisevener tonisevener self-assigned this Mar 31, 2022
Copy link
Collaborator

@tonisevener tonisevener left a comment

Choose a reason for hiding this comment

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

@mcleinman thanks for helping us out with this! I have a few suggestions, let me know what you think.

@@ -356,18 +357,18 @@ final class NotificationsCenterCell: UICollectionViewCell {
// Foreground and Background Container Constraints

NSLayoutConstraint.activate([
swipeBackgroundFillView.leadingAnchor.constraint(equalTo: contentView.leadingAnchor, constant: NotificationsCenterCell.swipeEdgeBuffer * 2.0),
swipeBackgroundFillView.leadingAnchor.constraint(equalTo: contentView.readableContentGuide.leadingAnchor, constant: NotificationsCenterCell.swipeEdgeBuffer * 2.0),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we adjust the placement of this readableContentGuide check elsewhere so that the swipe actions go all the way to the edge of the screen? This would line up better with how the saved articles cells act. Also it would be good if the background color when selected / highlighted can extend to the very edges too. We might want to confirm with @carolynlimadeo on this first.

Current PR:
IMG_1249 2
IMG_1248
IMG_1247

Ideally:
IMG_1246
IMG_1245
IMG_1244

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been fixed, thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mcleinman I'm seeing something odd when I first load the screen - the messageSummaryLabel appears squished at first, then expands out to the proper width after 1 second. I didn't notice it in main, so I think this is related to the readableContentGuide changes. I'm testing this on an iPad Pro 11-inch, iOS 15.2. I'm also seeing it on iPhone.

out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I couldn't isolate why this was happening - tried a few different things. Thus, I went to your way of making the readable content guide work - that seems to work great, as well as fix this problem. Thank you!

Wikipedia/Code/NotificationsCenterFilterView.swift Outdated Show resolved Hide resolved
Wikipedia/Code/NotificationsCenterCell.swift Show resolved Hide resolved
@tonisevener tonisevener removed their assignment Mar 31, 2022
@mcleinman
Copy link
Contributor Author

I believe this is ready for another review. Thanks to @tonisevener for her help with parts of this!

@tonisevener tonisevener self-assigned this Apr 14, 2022
@tonisevener
Copy link
Collaborator

@mcleinman Thanks - this is looking good! Just a couple of last things - hopefully they're an easy fix:

It looks like the non-Wikipedia project icons are still aligned to the edge. Screenshot from iOS15.2:

IMG_1251

This summary label bleeds too far into the project icon area. Oddly I don't see this behavior in the compact horizontal size class, it wraps well before the project icon. Which is good, but surprising since I don't think we're doing anything differently via code for that. Screenshot from iOS15.2:

IMG_C9EBA9EEA314-1

@tonisevener tonisevener removed their assignment Apr 14, 2022
@mcleinman
Copy link
Contributor Author

@tonisevener I think this is finally set. Fingers crossed. That last set was sloppy, this one should finally fix the readable content.

@tonisevener tonisevener self-assigned this Apr 15, 2022
Copy link
Collaborator

@tonisevener tonisevener left a comment

Choose a reason for hiding this comment

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

🎉 Working great!

@tonisevener tonisevener merged commit cfa2762 into main Apr 15, 2022
@tonisevener tonisevener deleted the T286607 branch April 15, 2022 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants