-
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
Posts & Pages: Integrate new cells in search #21828
Posts & Pages: Integrate new cells in search #21828
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.
|
79f9b89
to
adc64f9
Compare
882dc31
to
d342e7a
Compare
d342e7a
to
11445d8
Compare
@@ -1,6 +1,87 @@ | |||
{ | |||
"object": { | |||
"pins": [ | |||
{ | |||
"package": "AutomatticAbout", |
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.
Do you see the same package.resolved changes? Not sure if I should commit or remote those.
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.
I do. I think we're seeing these changes because they were deleted in bac6644
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.
If we revert the changes made in bac6644, we won't see package.resolved changes going forward, right?
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.
Hmm, OK let me revert that. I'll merge it with this PR.
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.
🤩
viewModel.$title.sink { [titleAndSnippetLabel] in | ||
titleAndSnippetLabel.attributedText = $0 | ||
}.store(in: &cancellables) |
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.
[nit] Wdyt of renaming viewModel.title to viewModel.content, and titleAndSnippetLabel to contentLabel?
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.
Yes, this is better – renamed.
f828c7e
to
2756f7c
Compare
This PR integrates new cells in search and adds live highlighting I tested the performance and there is no need to move the highlighting to the background.
Simulator.Screen.Recording.-.iPhone.15.-.2023-10-19.at.11.25.05.mp4
Regression Notes
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: