Fix media item details layout on iPad #22042

kean · 2023-11-15T12:48:45Z

It's a production issue, hence I'm targeting trunk.

To test:

For all media types: images, video, documents, and audio

Verify that the preview for are displayed using reasonable size on all devices and device orientations
Verify that the previews are loaded quickly (now use .medium thumbnail size)
Verify that when you tap on a preview, it displays the selected media item fullscreen

Regression Notes

Potential unintended areas of impact: Site Media Details
What I did to test those areas of impact (or what existing automated tests I relied on): manual
What automated tests I added (or what prevented me from doing so): n/a

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

Portrait and landscape orientations.
Light and dark modes.
Fonts: Larger, smaller and bold text.
High contrast.
VoiceOver.
Languages with large words or with letters/accents not frequently used in English.
Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
iPhone and iPad.
Multi-tasking: Split view and Slide over. (iPad)

peril-wordpress-mobile · 2023-11-15T12:48:51Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

wpmobilebot · 2023-11-15T13:22:41Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr22042-bf9d392
	Version	23.7
	Bundle ID	org.wordpress.alpha
	Commit	`bf9d392`
	App Center Build	WPiOS - One-Offs #7894

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2023-11-15T13:27:05Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr22042-bf9d392
	Version	23.7
	Bundle ID	com.jetpack.alpha
	Commit	`bf9d392`
	App Center Build	jetpack-installable-builds #6917

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

crazytonyli · 2023-11-16T00:43:42Z

WordPress/Classes/ViewRelated/Media/MediaItemViewController.swift

+    override func viewDidLayoutSubviews() {
+        super.viewDidLayoutSubviews()
+
+        headerMaxHeightConstraint.constant = view.bounds.height * 0.75


Why do we need to update the constant here? Can we create the constraint as something like:

headerMaxHeightConstraint = headerView.heightAnchor.constraintTo(lessOrEqualThan: view.heightAnchor, multiplier: 0.75)

I forgot multiplier was a thing. Unfortunately, it doesn't seem to work in this case maybe because the constraint system doesn't receive an updated view.height value until after the viewDidLayoutSubviews is completed, which is too late. You can reproduce it by running it on iPad and rotating the device from portrait to landscape.

crazytonyli · 2023-11-16T00:44:53Z

WordPress/Classes/ViewRelated/Media/MediaItemViewController.swift

@@ -247,7 +226,7 @@ class MediaItemViewController: UITableViewController {
            if isSharing {
                let indicator = UIActivityIndicatorView()
                indicator.startAnimating()
-                indicator.frame = CGRect(origin: .zero, size: CGSize(width: 44, height: 44))
+                indicator.frame = CGRect(origin: .zero, size: CGSize(width: 43, height: 44))


🤔 What's this change for?

It's to make sure the activity indicator has the same button (see the video). It's not great, but I couldn't quickly find a better solution and moved to other tasks.

Untitled.mp4

crazytonyli · 2023-11-16T00:59:18Z

WordPress/Classes/ViewRelated/Cells/MediaItemHeaderView.swift

+    private func setupAccessibility() {
+        accessibilityTraits = .button
+        accessibilityLabel = NSLocalizedString("Preview media", comment: "Accessibility label for media item preview for user's viewing an item in their media library")
+        accessibilityHint = NSLocalizedString("Tap to view media in full screen", comment: "Accessibility hint for media item preview for user's viewing an item in their media library")


Use reverse DNS keys? See p91TBi-aJl-p2

crazytonyli · 2023-11-16T01:03:26Z

WordPress/Classes/ViewRelated/Cells/MediaItemHeaderView.swift

+    override func layoutSubviews() {
+        super.layoutSubviews()
+
+        videoIconView.center = center


Nitpick: Seeing this view is centered differently to loadingIndicator and errorView, I'm guessing it's because PlayIconView doesn't support auto layout? If that's the case, maybe adding a comment here could avoid confusing why this view is done differently?

I tried, but PlayIconView does not use Auto Layout and it loses its default frame if you use it with constraint. I left a comment why it frame-based layout.

wpmobilebot · 2023-11-21T16:26:10Z

	1 Warning
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

kean added 5 commits November 14, 2023 18:00

Rework Media item header view

3e9a500

Switch to QLPreviewController for previews

5e098fc

Remove QLPreviewController

e781b8f

Remove unused code

d92babd

Improve layout of header view on iPad

46a6b7c

kean added [Type] Bug Media labels Nov 15, 2023

kean added this to the 23.8 milestone Nov 15, 2023

Update release notes

191b8ce

kean requested a review from crazytonyli November 15, 2023 12:49

Revert some of the changes

4157677

crazytonyli reviewed Nov 16, 2023

View reviewed changes

kean added 2 commits November 16, 2023 07:38

Minor updates based on feedback

7e391ee

Add localizable strings

38058ef

kean requested review from crazytonyli and momo-ozawa November 16, 2023 12:50

Merge branch 'trunk' into task/improve-media-item-view

bf9d392

crazytonyli approved these changes Nov 21, 2023

View reviewed changes

kean enabled auto-merge November 21, 2023 19:42

kean merged commit b6e761f into trunk Nov 21, 2023

kean deleted the task/improve-media-item-view branch November 21, 2023 22:05

kean mentioned this pull request Nov 28, 2023

Remove the MediaThumbnailService #22106

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix media item details layout on iPad #22042

Fix media item details layout on iPad #22042

kean commented Nov 15, 2023

peril-wordpress-mobile bot commented Nov 15, 2023

wpmobilebot commented Nov 15, 2023 •

edited

Loading

wpmobilebot commented Nov 15, 2023 •

edited

Loading

crazytonyli Nov 16, 2023

kean Nov 16, 2023

crazytonyli Nov 16, 2023

kean Nov 16, 2023

crazytonyli Nov 16, 2023

kean Nov 16, 2023

crazytonyli Nov 16, 2023

kean Nov 16, 2023

wpmobilebot commented Nov 21, 2023

Fix media item details layout on iPad #22042

Fix media item details layout on iPad #22042

Conversation

kean commented Nov 15, 2023

Regression Notes

peril-wordpress-mobile bot commented Nov 15, 2023

wpmobilebot commented Nov 15, 2023 • edited Loading

wpmobilebot commented Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpmobilebot commented Nov 21, 2023

wpmobilebot commented Nov 15, 2023 •

edited

Loading

wpmobilebot commented Nov 15, 2023 •

edited

Loading