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 List: Add Copy link functionality for posts #15570
Posts List: Add Copy link functionality for posts #15570
Conversation
WordPress/src/main/java/org/wordpress/android/viewmodel/posts/PostListItemUiStateHelper.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/viewmodel/posts/PostListItemUiStateHelper.kt
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/posts/PostListAction.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/posts/PostListAction.kt
Show resolved
Hide resolved
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.
Great work @ovitrif 👍
I've tested the modified functionality and it works great. The code and the tests also looks good 🎉
I've added a few comments/suggestions but nothing crucial :)
Many thanks @antonis for your detailed and careful review, I'm making this pull-request ready for review and no longer a draft, and removing the [DO NOT MERGE] tag. We still need a designer's point of view from @iamthomasbishop on the solution for the issue we're addressing here 🙇🏻♂️ Feedback is always welcome! |
@ovitrif This looks great, I just have a couple of concerns:
Maybe I'm missing context because I haven't seen all of the previous discussion—is there a reason we're not showing the
Would it be possible to add a divider above the "Trashed" menu option? (Note: This could be tackled separately because it's a pre-existing concern, not a result of the work here)
I think we've moved away from using Toasts in favor of Snackbars for this type of notice, so that would be preferred here. On a related note, I would also implement this as a Snackbar on iOS (which is a custom component IIRC; named either "Snackbar" or "Notice") if that's in scope—if not, would you mind creating a sibling implementation ticket over on the WPiOS repo and we can triage it in the near future? |
Thank you for your feedback @iamthomasbishop 🙏
I missed that Thomas. I think it would be good to align with the web on this if it is possible 👍
I agree that this can be tackled on a different PR later.
Good catch 👍
I'll take a note to create a ticket on this to tackle separately. |
Hi @iamthomasbishop, thanks a lot for the feedback 🙇🏻♂️!
It is true that we show it also for posts/pages in "Draft" and "Scheduled" states on the web, we can definitely align the app in this regard!
Definitely a nice idea! I agree that we can handle the divider in a separate task.
Thank you for the information, with that in mind indeed a snackbar should be the preferred solution here.
Noted! @antonis will take care of creating the ticket for the WPiOS repo. |
WordPress/src/main/java/org/wordpress/android/viewmodel/posts/PostListItemUiStateHelper.kt
Show resolved
Hide resolved
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
WordPress/src/main/java/org/wordpress/android/ui/posts/PostListAction.kt
Outdated
Show resolved
Hide resolved
You can test the changes on this Pull Request by downloading the APKs: |
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.
Great work @ovitrif 👍
I retested the changes with the latest commits and everything works as expected 🎉
The code also LGTM 🚢
Fixes #13292
Description
This PR implements the "Copy link" functionality for published posts (already exists on Calypso).
We've received a request in beta tester feedback to have an option to copy the URL for a post:
The proposed solution is inspired from the web version, where:
Preview web version
To test
Published/Scheduled/Draft Posts
Trashed Posts
Screenshots
Regression Notes
Potential unintended areas of impact
Buttons for non-published posts
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
Unit testing
What automated tests I added (or what prevented me from doing so)
n/a
PR submission checklist:
RELEASE-NOTES.txt
if necessary.