Add Media File and Attachment Page options to the native Image block Link To menu #34846
Conversation
Displays "Media" and "Attachment Page" options within the Link To settings for the Image block. The logic does not work for the new options, and the implementation is rudimentary. Several `TODO(David)` are in place for remaining work.
Using a callback avoids placing context-specific props reference inside of link settings. I.e. `imageUrl` and `attachmentPageUrl` are no longer referenced within link settings.
Reusing label text from the web will likely increase familiarity and reduce confusion for users.
Display the currently selected link to option in the top-level Image block settings.
e3a1710
to
717cf6c
Compare
|
@mkevins this is very much still a work-in-progress and not completely functional, but I wanted to share it in case you'd like to provide a high-level review on the overall approach. A review now is not urgent if you do not have time, I will definitely seek your full review before merging this work. Thanks! |
|
Size Change: +100 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
Relying upon the `placeholder` did not work correctly whenever a `value` was set, i.e. in the case of a Custom URL. Additionally, overloading the `placeholder` prop is confusing. The new `valueMask` prop hopefully better communicates the intent.
Avoid superfluous import.
The prior references are non-existent.
Selecting Media File or Attachment Page sets a URL value. Originally, this caused the Custom URL option to display the URL value, rather than persistently displaying its "Search or type URL" placeholder. The Custom URL option should only display an actual URL value if it is manually typed or selected within the link picker.
When the Media File or Attachment Page options are selected, a URL value is set. Previously, this value was passed to the link picker when the Custom URL option was subsequently selected. This meant the link picker would be pre-populated with a (likely unidentifiable) URL that was not manually entered by the user, which could be confusing.
94e5afd
to
648780e
Compare
Referencing `inputValue` from within LinkSettingsScreen resulted in a stale value to from `useRoute`. By referencing, the `url` where the callback is defined, we avoid this issue.
Set more appropriate defaults for link settings, the new values match the sibling state values of the components. Previously the `undefined` values for `label` and `rel` differed from the empty string values in component state. This change helps avoid unnecessary calls to `setAttributes` that occurred when the BottomSheet was closed. In addition to being unnecessary, these additional `setAttributes` calls caused the Image edit component to erroneously change the `linkDestination` even though the URL had not changed.
b693fc8
to
d9296cd
Compare
When "Open in new tab" or "Link Rel" is modified, the URL is again passed to be set within `setMappedAttributes`. This caused the `linkDestination` to improperly be modified. This change now avoid settings `linkDestination` if the URL is not changing.
External images will not have an attachment page URL, so this option should not be displayed when the URL is undefined.
Toggling the icon opacity rather than the presence of the icon itself produces the desired alignment for both selected an unselected options.
After reviewing the logic within `LinkSettings`, it largely manages invoking `setAttributes` in different scenarios. Therefore, using `setAttributes` directly makes sense for this component.
Attempt to better describe component intent.
Relocate the logic to allow for a single `isSelected` prop to improve readability.
|
@mkevins thank you for the review. 🙇🏻 I have addressed each of your comments. When you have time, will you please review again and let me know if there is anything we'd like to address further? |
|
Thanks for addressing all the comments!
I've given it another pass, and it is looking great! Also, I tested it out, and it is mostly working according to the steps described, with a few exceptions: For some reason, at one point I got into a state where the media file option would not persist. The steps are as follows:
When I did this, the media file was not set, and instead it remained at none. 🤔 media-file-not-persisted.mp4When I set the media URL to an external image (via the web) I happened to have the attachment page setting already set. This resulted in the attachment page appearing to be set, but it was not present in the options (as expected). attachment-page-still-set.mp4I'm not sure if this is something we can easily address (and may likely be an issue for web as well). Also, it's a bit "out of the way" to reproduce it, so I wouldn't expect the impact to be large for this issue. Just noting it since I happened to encounter this while testing that flow. Finally (though this is not introduced in this PR, and I'm only mentioning for completeness), I also encountered the strange "transparent fullscreen modal" again:
|
@mkevins I observed an issue like this in the past, but I am been unable to reproduce this now. 😕 May I verify you are using the latest build for Android:
From my testing, it does appear the web suffers from the same "stale" link issue. However, it is not as pronounced as the web UI merely renders the "stale" link address rather than claiming it to be the image's "Attachment Page" specifically. Web UI: Image with "detached" Attachment Page URL
One fix might be no longer relying solely upon I would agree—with what I believe you implied—that we could ship with this issue as it is likely an edge case.
Yes, hopefully we are able to find a solution to this issue within #34425. |
Actually, I encountered this issue by testing this PR and using WordPress-Android develop (at this commit: wordpress-mobile/WordPress-Android@9a6f115). It was with a new image that I created by inserting an image block and choosing an image from the media library. I did not close the bottomsheet between switching link destinations, nor update the post between switching options. I will also try with the generated APK to see if there's different behavior there.
Yes, you believe right 😄 : I don't consider the second issue to be a blocker for merging the PR. |
|
I was able to reproduce it on the apk as well (though it seems I had to fiddle quite a bit by changing the options a lot — I even added custom URL twice). I'm not sure why sometimes it happens with fewer steps 🤔 media-file-not-persisted-apk-build-115665.mp4You may also notice in that video a regression unrelated to this PR in which the search results are cleared a moment after they are displayed, which seems as though it might be related with the keyboard suggestion, since it happens to me when I swipe a word, but not hunt-and-peck 🤷♂️ . I wonder whether these kinds of state issues could be related with the bottomsheet animations / navigation 🤔 |
There are no references to this method in the source.
This prop referenced an undefined class method.
Stringifying the styles appears unnecessary, and it was also causing missing key warnings in tests as the styles are undefined in that context. Leveraging the index appears to be enough to satisfy both the app and the tests.
Test from the "public" API of the component and its output, rather than internal methods. I.e. press rendered buttons instead of calling methods by name.
Metro resolves native files by default.
The Image component invokes `getMedia` within its selector. This results in an async resolver running, which leads to changes to the component after the async work completes. Rather than wrapping the entire test helper with `act`, we should target the specific change within the test file.
Per React Navigation documentation, this file should be included. https://bit.ly/3n6Rotv
There appears to be a bug in React Navigation that causes error logs in Jest tests. We must await the `fireEvent` that triggers the navigation event in order to suppress the error. https://git.io/Ju35Z
Originally, ImageLinkDestinationsScreen relied upon (1) differing route parameter names from LinkPicker and (2) explicitly set `linkDestination` while LinkPicker did not. Stale route parameters resulted in erroneous attribute updates when swapping the link destination from Media File > Custom URL > Media File. This change addresses the stale route parameters by (1) unifying the route parameter names between the two components and (2) unifying the approach for setting `linkDestination` to be computed based upon the URL. The URL, represented as the `inputValue` route parameter, is now the sole route parameter.
…ink-image-to-media
The image URL may contain query parameters for aspects like display dimensions. This caused a comparison based upon the URL to unexpectedly mismatch. By leveraging `linkDestination` instead, we correctly hide the URL from the Custom URL input whenever Media File or Attachment Page is selected.
|
@mkevins thank you for your patience while I found time to address your feedback. 🙇🏻 This PR and its sibling are now ready for another round of review. Installable builds can be found on the sibling WPiOS and WPAndroid PRs. I ended up cherry picking a few commits from another branch to allow me to write tests from this new Image work. So, it may be easiest to step through each of the commits since your last review individually. I tried to leave commit messages explaining each. (FIXED) Selection Sporadically Not PersistingI believe I have addressed this issue within cf4027f. To recap, here are the steps I used to reproduce the issue:
(POSTPONED) Incorrect Link To After Swap to External ImageWe agreed that we do no need to address this issue within this PR, but I will recap in case we decide to re-engage it later. We noticed the web editor seemingly suffers from a stale URL in this context as well, but it is not as noticeable. For future reference these lines appear to be relevant, and were modified from the previous code that managed this scenario. I will create a new issue for this if/when we merge this work.
|
Related PRs
Description
Fixes wordpress-mobile/gutenberg-mobile#924. Add Media File and Attachment Page options to the native Image block Link To menu to allow quickly setting commonly used URLs. This feature already exists in the web interface.
Installable builds are available on the aforementioned PRs.
How has this been tested?
Verify Link Destinations
Verify External Image Options
Screenshots
Screenshots
Video
ios-image-link-destinations.MP4
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).