1094202 - Implement the Share To Extension UI

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Design at http://cl.ly/YMMo - Robin, How final is this? Can we go ahead with this design for the Share Extension for the portland demo?

Comment 1

[Robin Andersen [:tecgirl]]

Stefan, it's close. The Share widget is good. The Action widget will be styled similarly, with device icons. Press on with these styles and I'll attach my latest iteration shortly.

Comment 2

[Robin Andersen [:tecgirl]]

Here is the updated Share/Action widget: http://cl.ly/YNax. I am still working on what the 'sent' notification will look/behave like. For now use the default indicator.

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Pull Request

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 8525368 [details] [review]
Pull Request

I think this is mostly with with comments addressed. I'm curious why you didn't use Snappy for defining any of your layout constraints!

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Comment on attachment 8525368 [details] [review]
> Pull Request
> 
> I think this is mostly with with comments addressed. I'm curious why you
> didn't use Snappy for defining any of your layout constraints!

File a followup for Snappy?

Comment 6

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> (In reply to Brian Nicholson (:bnicholson) from comment #4)
> > Comment on attachment 8525368 [details] [review]
> > Pull Request
> > 
> > I think this is mostly with with comments addressed. I'm curious why you
> > didn't use Snappy for defining any of your layout constraints!

Err, s/with with/fine with/.

> File a followup for Snappy?

Sounds like a good idea. Filed bug 1102516.

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I really don't know if Snappy is the way to go. I like the concept but it is an unmaintained and poorly documented third party library. It has open issues and pull requests that the author has not dealt with in months.

What is wrong with the visual language that UIKit provides? It looks pretty simple no? I'd rather depend on Apple's stable and well documented APIs. AutoLayout is already a pretty tough subject to master. So using standard APIs lets us built on common knowledge/patterns from books/articles/StackOverflow.

Comment 8

[Brian Nicholson (:bnicholson)]

(In reply to Stefan Arentz [:st3fan] from comment #7)
> I really don't know if Snappy is the way to go. I like the concept but it is
> an unmaintained and poorly documented third party library. It has open
> issues and pull requests that the author has not dealt with in months.

Not sure what you mean by unmaintained -- the last change was less than a month ago. The open issues seem minor, and he's commented on them recently.

> What is wrong with the visual language that UIKit provides? It looks pretty
> simple no? I'd rather depend on Apple's stable and well documented APIs.

Well, take a look at Snappy's GitHub page (https://github.com/Masonry/Snappy) and compare the before/after. I wouldn't say it's simple at all; the Snappy equivalent is much, much easier to read IMO. The visual language string itself isn't too complicated, but with all of the additional parameters, and the fact that the visual language can't handle centering and needs additional defined constraints, the code quickly becomes overwhelming.

Did you actually run into any issues with Snappy? I agree it's unsettling to work with an evolving API (though Swift itself is evolving), but the project is open-source, so we can fix any issues ourselves (if we find any). If you look through the Snappy codebase, it's actually quite small.

Comment 9

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

How about we convert to Snappy in the next iteration of this code. I'd like to wait until UX has a better idea of that this dialog is supposed to look like as things seem to be rapidly changing at this point.

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Reopening this because this is not functional at this point and it has a number of open bugs that need to be resolved first.

Comment 11

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: PR: https://github.com/mozilla/firefox-ios/pull/279

This patch brings the Share To extension UI up to date with the latest specification. There are a couple of items though:

* I am reusing icons (login in navigation bar, bookmark, add to reading list) from the application icon set and the reading list icon set. I can either scale these down if needed, or we can incorporate new sizes for this dialog.
* I've left some of the fonts to the system default. Specifically the navigation bar buttons and the table row label. I don't know if we should actually change the size of those.
* Note that the dialog has a dynamic size. It's maximum width is 380px. But it also enforces a 16px padding left and right. So on small width devices like the 4S and 5, the max width is 288px. (This is done with auto layout rules, so we can change these dynamics)

Tested on iPhone 4S, iPhone 5, 6, 6 Plus and iPad. Fits & looks good on all these device sizes.

This is not pixel/spec perfect, but it is easy to change paddings, font sizes, colors and item sizes by editing the `ShareViewController.swift` file and changing the settings in the `ShareDialogControllerUX` struct.

Comment 12

[Robin Andersen [:tecgirl]]

Comment on attachment 8583995 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/279

Stefan, It looks good! I made some updates to your branch (Bug1094202ShareToUI), mainly bumping up the app icon a bit, bumping up the font-size, and lessening some padding.

Comment 13

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I merged this, r+ by me.

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 8583995 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/279

I thought I r+ this before.