Closed Bug 1094202 Opened 7 years ago Closed 7 years ago

Implement the Share To Extension UI

Categories

(Firefox for iOS :: General, defect)

All
iOS 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: st3fan, Assigned: st3fan)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
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.
Summary: Implement the Share Sheet UI → (ios) Implement the Share Sheet UI
Summary: (ios) Implement the Share Sheet UI → (ios) Implement the Share To Extension UI
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.
Status: NEW → ASSIGNED
Attached file Pull Request (obsolete) —
Attachment #8525368 - Flags: review?(wjohnston)
Attachment #8525368 - Flags: review?(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!
Attachment #8525368 - Flags: review?(bnicholson) → review+
(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?
Depends on: 1102516
(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.
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.
(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.
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.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 1104118
Depends on: 1104120
Depends on: 1104123
Group: mozilla-employee-confidential
OS: Mac OS X → iOS 7
Product: Firefox for Android → Firefox for iOS
Hardware: x86 → All
Version: Trunk → unspecified
Summary: (ios) Implement the Share To Extension UI → Implement the Share To Extension UI
Reopening this because this is not functional at this point and it has a number of open bugs that need to be resolved first.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1136236
Attachment #8525368 - Flags: review?(wjohnston)
No longer depends on: 1102516
No longer depends on: 1104120
No longer depends on: 1136236
No longer depends on: 1104123
No longer depends on: 1104118
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.
Attachment #8583995 - Flags: feedback?(randersen)
Attachment #8525368 - Attachment is obsolete: true
Attachment #8583995 - Flags: review?(wjohnston)
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.
I merged this, r+ by me.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Comment on attachment 8583995 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/279

I thought I r+ this before.
Attachment #8583995 - Flags: review?(wjohnston)
You need to log in before you can comment on or make changes to this bug.