-
-
Notifications
You must be signed in to change notification settings - Fork 643
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
[android] Multiple share to zulip / share to Zulip from other apps [part 3] #4905
Conversation
@gnprice here is the PR for multi-share to zulip. |
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.
Thanks @AkashDhiman ! This generally looks great -- some comments below.
I'll take a crack at those SharedData
types and see what I can come up with that I like.
src/sharing/ShareWrapper.js
Outdated
overlayPosition="bottom-right" | ||
overlay={<IconCancel color="gray" size={20} onPress={() => this.deleteItem(item)} />} | ||
> | ||
{item.mimeType.startsWith('image') ? ( |
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.
{item.mimeType.startsWith('image') ? ( | |
{item.mimeType.startsWith('image/') ? ( |
Including the delimiter means it doesn't accidentally match some other word that happens to start with the same letters.
(Which isn't super likely in this case, but is good in general so as not to have to think about the possibility.)
if (sharedData.type === 'image' || sharedData.type === 'file') { | ||
const url = | ||
sharedData.type === 'image' ? sharedData.sharedImageUrl : sharedData.sharedFileUrl; |
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.
I like this prep commit simplifying this code!
src/types.js
Outdated
export type File = {| | ||
name: string, | ||
mimeType: string, | ||
url: string, |
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.
This is a very general name, and it's here in the global types.js
so it has a very general scope. But the real meaning isn't so general -- this is really specific to having a file shared with us (and for that matter specific to Android's API for that; I'm not sure what it looks like on iOS, and we'll figure that out when we add the feature there.)
SharedFile
would be a good name, except that it's taken (and even at the tip of the branch, it'd be confusing to have it look so parallel to SharedText
when it isn't.)
I may try doing a quick refactor of these types and pushing that to the PR branch.
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.
(done -- see below in main thread)
android/app/src/main/java/com/zulipmobile/sharing/SharingHelper.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/com/zulipmobile/sharing/SharingHelper.kt
Outdated
Show resolved
Hide resolved
val file = urlToSharedFile(url, contentResolver) | ||
files.pushMap(file) |
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.
Could tighten a bit by inlining file
here -- the resulting line is only slightly longer.
<FlatList | ||
data={this.state.files} | ||
renderItem={this.renderItem} | ||
keyExtractor={i => i.url} | ||
horizontal |
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.
Horizontal sounds good here. Does it scroll if the previews don't fit in the width of the screen?
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.
Yes.
I just tested this, by temporarily setting the image width > my screen width, since images have fixed size of 200 x 200 which is smaller than most practical device widths; observation was that single image scrolls if its width exceeds screen width.
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. What if you have several images, so that each one fits but combined they don't?
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.
Then the flatlist scrolls horizontally
src/sharing/ShareWrapper.js
Outdated
overlaySize={20} | ||
overlayColor="white" | ||
overlayPosition="bottom-right" | ||
overlay={<IconCancel color="gray" size={20} onPress={() => this.deleteItem(item)} />} |
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.
Buttons should be at least size 24. (Do we have this in, or linked from, our style guide somewhere? If not, we should.)
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.
changed overlaySize and IconCancel
's size to 24.
I couldn't find a style guide that said 24, and referred to the size from similar code in AvatarItem
which is 20. Should that be changed as well?
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.
Hmm, good question.
So, actually the most important aspect of this is that the touch target should be at least 48px, to make it accessible for people to accurately hit:
https://material.io/design/usability/accessibility.html#layout-and-typography
(I just went and looked that up, and it reminded me that the needed touch-target size is that big.)
Looking at AvatarItem
, the whole avatar is the touch target, and it's 50px across. (You can see this in the "Group PM" / "New group PM" screen: select some users, then touch anywhere on their avatar to remove them.) So we're covered there. That also means that the icon isn't really functioning as a button: it's more like a label on the whole avatar as a button, indicating what touching it will do.
Here, I think making the whole 200px preview a touch target for removing it would be counterintuitive. But perhaps we can make a whole 48px square in the corner around the "cancel" icon be the touch target?
ec7c7a0
to
e0e790f
Compare
OK, just added some prep commits to clean up the (I also marked a couple of the commits as needing commit-message updates following the rebase; I resolved code conflicts, but didn't update the commit messages.) |
e0e790f
to
2a218e8
Compare
@gnprice made the changes you requested and replied to your questions above, you may take a look. |
2a218e8
to
cd95bf3
Compare
We don't ever use these names, outside of defining `SharedData` itself.
This type definition belongs squarely to a nice well-defined subsystem, and we have a nice subdirectory for that subsystem. So put the definition there, alongside the other (JS) code belonging to that subsystem. This is also good for reducing cycles in our type-import graph.
This lets us not give names to the individual branches of this union, which frees up namespace so that in particular we can use a name like `SharedFile` for each file's data when we add support for multiple files in `SharedData`. This also makes it easier to take in the whole definition at once, since it's short enough that it fits so compactly this way.
The admonition to keep these in sync with platform-native code is addressed to someone editing this definition, rather than someone looking at its interface in order to use it elsewhere, so it naturally goes in an implementation comment (with `//` or plain `/* … */`) rather than a jsdoc comment (with `/** … */`). Putting it more immediately next to the very implementation also improves the chances that someone editing the latter will actually notice the comment and act on it. Also add a quick note about iOS to go with the note about the situation on Android.
Done to be better compatible with multiple file sharing (which will be introduced later on). Structuring it this way will help us to later on bundle files of all types inside a single property `files` of `SharedData` type where an individual file type is identified using it's mimeType. `mimeType` is taken out using contentResolver.getType, in case the mimeType is unknown it returns null, to handle that we send a mimetype for denoting binary data (application/octet-stream) when it does return null. The overall change makes the code more adaptive since we would not have to create more types for other files like SharedAudio, SharedImage each time we try to add custom functionality for such types in the future. Associated code is changed accordingly to accomodate this change.
This commit does the following * Updates `SharedData` replacing subproperty `file` with an array of `file` called `files`. * Changes the logic to contruct `SharedData` in kotlin to accomodate `MULTIPLE_SEND` intent. * Updates the preview of shared files to display a flatlist of all of them. * Enables intent filter for `MULTIPLE_SEND`.
cd95bf3
to
ec7c09b
Compare
Thanks for the revision! I have a couple of comments above about the UI, but this is definitely a major improvement to the UI already. So I'm merging, and those can be followups. I made one commit-message tweak:
just to make the summary line somewhat more informative about what's happening. Those comments above are: |
This PR makes changes to enable users to share multiple files.
Followup for #4757