-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Notifications P1] Add missing notification selection color for a split screen #22686
[Notifications P1] Add missing notification selection color for a split screen #22686
Conversation
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
fe31a6c
to
aab45b1
Compare
enum Style { | ||
case leading | ||
case subsequent | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to name this to something more elaborate? I couldn't understand what it is without reading its usage. Also could you move this enum to the top of the class right below the class signature (unless you're against it)? For public nested types, it is common to list them on the top so when a person views the file, all relevant types are at the top rather than being scattered through the file.
enum ... {
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally makes sense! Will try to find more accurate naming.
...ess/Classes/ViewRelated/Views/List/NotificationsList/NotificationsTableViewCellContent.swift
Outdated
Show resolved
Hide resolved
if splitViewControllerIsHorizontallyCompact { | ||
cell.selectionStyle = .none | ||
} else { | ||
cell.selectionStyle = .default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are enabling it for all cases except for compact split view, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. It's also disabled for the split mode on iPad:
Simulator.Screen.Recording.-.iPad.Pro.12.9-inch.6th.generation.-.2024-02-22.at.15.26.30.mp4
Thanks for the nice fix! I've also noticed that the content's position in the cell wasn't centered. Even if selectionStyle would be none, this is a nice improvement. |
…ssing-notification-selection-color
…ssing-notification-selection-color
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG2M
Fixes #22672
This PR fixes the lack of the notification rows' selection style on iPad (during split notifications mode). I also modified the rows' paddings to make the selection look natural. iPad multitasking mode is also handled to switch the selection style accordingly to the screen's width.
Selection with no padding changes
To test:
Regression Notes
Potential unintended areas of impact
Notifications screen
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual tests, UI tests
What automated tests I added (or what prevented me from doing so)
UI tests already exist
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: