-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update user details view #23777
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
Update user details view #23777
Conversation
|
| App Name | WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23777-c8778ba | |
| Version | 25.4.2 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | c8778ba | |
| App Center Build | WPiOS - One-Offs #11009 |
|
| App Name | Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23777-c8778ba | |
| Version | 25.4.2 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | c8778ba | |
| App Center Build | jetpack-installable-builds #10049 |
Generated by 🚫 Danger |
d876f7d to
5fbfd0c
Compare
| } | ||
| ) | ||
| if let url { | ||
| AsyncImage( |
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.
Please replace this with AvatarView. AsyncImage should basically never be used.
AvatarView or AsyncImageView are currently unavailable in WordPressUI as they are part of the app target. But UserProfile* don't belong to WordPressUI as it's designed for reusable components. It should be small and should compile fast as, in the long term, more modules will depend on it.
I suggest the following changes:
- Create a
WordPressMediaframework (or similar name) withImageDownloader– other media-related stuff could go there later too. - Move
AvatarViewandAsyncImageViewto eitherWordPressUIorWordPressMedia - (If you want to keep the previews) Create a separate module for user management.
If we keep adding everything to WordPressUI, it will eventually stop working well for Xcode Previews as these tend to become unusable with 10K+ lines in a module. The only long-term option if we want to facilitate Xcode Previews is to create feature-based modules (aka app micro modules). Since we are now fully in the SPM world, it should be feasible, so it might be a good time to start.
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.
Sorry, I forgot to reply to your original comment about AsyncImage.
Shall I just move the new user management code to the app target for now? So that the AvatarView and ImageDownloader stuff (I don't think we need it for this particular feature) will be available. /cc @jkmassel
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.
Let's address it separately. I'm going to add the approval as is.
|
|
||
| let dummyDataUrl = URL(string: "https://my.api.mockaroo.com/users.json?key=067c9730")! | ||
| enum Scenario { | ||
| case inifitLoading |
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.
typo – "infinite"
|
|
||
| if viewModel.currentUserCanModifyUsers { | ||
| Section(Strings.accountManagementSectionTitle) { | ||
| NavigationLink { |
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.
Let's show these as modal screens.
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.
In follow-up PRs, I'll use an alert for the "set new password" feature and a modal/sheet for deleting users.
|
@kean I couldn't reproduce that crash, neither on an iPad nor on an iPad simulator. Can you produce it consistently? I wonder if that has something to do with the new window (instead of an existing window from a view controller) used here: Line 222 in 2d76a25
Note: when passing |
5fbfd0c to
2caa0ae
Compare
|
⬆️ Rebased without any code changes. |
| } | ||
| ) | ||
| if let url { | ||
| AsyncImage( |
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.
Let's address it separately. I'm going to add the approval as is.



Note
This PR is built on top of #23768.
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txtif necessary.Testing checklist: