Skip to content

Add PostAuthorPicker - #24621

Merged
kean merged 3 commits into
feature/post-settingsfrom
task/rewrite-author-selector
Jun 26, 2025
Merged

Add PostAuthorPicker#24621
kean merged 3 commits into
feature/post-settingsfrom
task/rewrite-author-selector

Conversation

@kean

@kean kean commented Jun 25, 2025

Copy link
Copy Markdown
Contributor

@kean
kean force-pushed the task/rewrite-author-selector branch from 7e657d7 to b418b3e Compare June 25, 2025 01:12
@kean kean added this to the 26.0 milestone Jun 25, 2025
@kean
kean requested a review from crazytonyli June 25, 2025 01:12
@wpmobilebot

wpmobilebot commented Jun 25, 2025

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
ConfigurationRelease-Alpha
Build Number28021
VersionPR #24621
Bundle IDcom.jetpack.alpha
Commit2afc43f
Installation URL2u4qlkt7lvekg
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot

wpmobilebot commented Jun 25, 2025

Copy link
Copy Markdown
Contributor
App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
ConfigurationRelease-Alpha
Build Number28021
VersionPR #24621
Bundle IDorg.wordpress.alpha
Commit2afc43f
Installation URL0nnil28gmo6ao
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

}

#if DEBUG
struct PostAuthorPicker_Previews: PreviewProvider {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason not to use #Preview?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed it – these previews in the app targets are not useful anyway.

@Environment(\.dismiss) private var dismiss

init(post: AbstractPost, onSelection: @escaping () -> Void) {
_viewModel = StateObject(wrappedValue: PostAuthorPickerViewModel(post: post, onSelection: onSelection))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I presume this is generated by AI? I feel bad about annoying you with the same comments. Any chance we can tell AI not to generate code like this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should rethink its usage. It's a standard officially documented pattern and there are only a few scenarios where it can lead to issues. It's too convenient not to use because it's the only good way to inject observable state into a view and have it retain it (state/stateObject). In this case, it's not issue because it's a new screen every time and a new view identify, to it rests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are multiple precedents in the codebase where StateObject(wrappedValue:) is used. I'd continue using it where it's OK – if it's part of the new screen it's fine.

@kean kean mentioned this pull request Jun 26, 2025
@kean
kean changed the base branch from trunk to feature/post-settings June 26, 2025 14:14
@kean
kean merged commit 2ec06c7 into feature/post-settings Jun 26, 2025
0 of 2 checks passed
@kean
kean deleted the task/rewrite-author-selector branch June 26, 2025 14:14
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants