-
Notifications
You must be signed in to change notification settings - Fork 301
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
Improvements to collaboration UI #1510
Conversation
Since we are handling the empty view of the collaborators list with an empty view, the Shared With message is now the header of a recycler view and the data items are transformed to CollaboratorDataItem to submit the list items.
You can test the changes on this Pull Request by downloading the APK here. |
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.
👋 @danilo04 !
I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟
I have left a test related question (❓), a suggestion (💡) and a few minor (🔍) comments for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.
PS: I was seeing a lot of issues that could have been fixed prior creating a commit if maybe Reformat code
, Rearrange code
and Optimize imports
were enabled. I am also not sure if your AS settings differ from mine. Can we sync on that so I can understand why that is indeed happening and maybe help you out avoiding those in the future?
Simplenote/src/main/java/com/automattic/simplenote/authentication/SessionManager.kt
Show resolved
Hide resolved
Simplenote/src/main/java/com/automattic/simplenote/authentication/SessionManager.kt
Outdated
Show resolved
Hide resolved
Simplenote/src/main/java/com/automattic/simplenote/CollaboratorsActivity.kt
Outdated
Show resolved
Hide resolved
Howdy 👋 @ParaskP7. Thanks for the review. Let me address your comments
Definitely, I will enable these options because without it is it a lot of work to check that in every commit. |
Looks good! Just a couple of things:
|
Hi @SylvesterWilmott, thanks for the review.
Fixed in 98189e5.
Fixed in 251e7e1. |
Avoid hardcoded parameters in method calls and improve code quality
Looks good! Thanks @danilo04 |
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.
👋 @danilo04 !
All LGTM, thanks for applying mine and Sylvester's suggestions, still approved on my part! 🥇
Fixes #1508
Fix
This PR adds a series of improvements asked in #1508. Those improvements include:
Test
Accept
. Should update the layout to list of collaboratorsYou cannot add yourself as a collaborator
Review
Only one developer and one designer are required to review these changes, but anyone can perform the review.
Release
These changes do not require release notes.