Add 2 static quick actions to Firefox for iOS. The static quick actions are: New Tab - opens to a new normal tab in Firefox New Private Tab - opens to a new private tab in Firefox
FYI there's an open PR from a contributor that adds these - maybe we can work with that? https://github.com/mozilla/firefox-ios/pull/1104
Ah, thanks. That's certainly something we can base this work off, although :tecgirl's design doesn't call for the show bookmarks/reading list but adds some dynamic items instead.
Created attachment 8690795 [details] [review] Pull request
Comment on attachment 8690795 [details] [review] Pull request Code looks good - just some nits. Also left an issue I experienced with testing this in the simulator using the simulator plugin. Not sure if it's an actual bug or not though.
Attachment #8690795 - Flags: review?(sleroux) → review+
Comment on attachment 8690795 [details] [review] Pull request I believe I see some commentary on github about this, but when I tap on New Private Tab, the URLbar and tab counter UI is off, http://c.tecgirl.com/dtoJ In a similar (yet inverted) fashion, this is what I see when tapping on New Tab: http://c.tecgirl.com/dtqO Additionally, the keyboard should be enabled for New Tab/New Private Tab, ready for the user to enter something. I will also add the correct assets for all instances, since I see we're just reusing what we have and they don't match. :)
Attachment #8690795 - Flags: ui-review?(randersen) → ui-review-
Created attachment 8691364 [details] [review] Pull request Fixed a number of issues. Asking for another review as quite a bit has changed. 1. Fixed crash when opening tab from cold This needed to be done asynchronously otherwise BVC didn't actually exist when tab was created causing crash 2. Fixed failure to apply UIAppearanceTheme It seems that once you have applied UIAppearance values to a component, any changes made to those UIAppearance values will not be applied on those components until you remove and then re-add them to a view. Therefore I am forcibly applying UIAppearance themes when starting from a QuickAction. 3. Fixed crash when adding new tab with different mode when tab tray open Found a crasher whereby if you opened a tab in a different mode to the one currently in effect in the backgrounded app, while the app is in the TabTray then the collection view works out the incorrect number of cells and crashes when it doesn't match. Fixed by applying new theme just like a tap to the PB button when a collectionView already exists in the TabTray, rather than just updating the privateMode. 4. Feedback from @tecgirl and now new tabs open with the URL bar as first responder.
Also, still waiting for correct assets from :tecgirl for QuickActions
Comment on attachment 8691364 [details] [review] Pull request LGTM - just a nit. I can't seem to test it this morning in the simulator though (this simulator hack isn't working for some reason :( ) As a side note, seems like using the UIAppearance proxy for theming isn't the best choice for what we want. Looks like it's more for setting up a static theme at the beginning of the app whereas we're using it to dynamically update styles. I'll file a bug to investigate a more robust/dynamic way of theming.
Attachment #8691364 - Flags: review?(sleroux) → review+
Bug filed for themes: https://bugzilla.mozilla.org/show_bug.cgi?id=1227550
Attachment #8691364 - Flags: ui-review?(randersen) → feedback?(randersen)
(In reply to Emily Toop (:fluffyemily) from comment #7) > Also, still waiting for correct assets from :tecgirl for QuickActions I added these to the meta so as not to have to separate them: https://bugzilla.mozilla.org/attachment.cgi?id=8691486
Comment on attachment 8691364 [details] [review] Pull request Great work, Emily! Note I dropped all the assets on the meta so they're in one place.
Attachment #8691364 - Flags: feedback?(randersen) → feedback+
Comment on attachment 8691364 [details] [review] Pull request Added assets. Just need a quick +1 from you Robin.
Attachment #8691364 - Flags: ui-review?(randersen) → ui-review+
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Both added strings are missing localization notes, https://github.com/splewako/firefox-ios-l10n-en/commit/6573867f993d2cb6c6a36f01d9f8be8ee01eb725
You need to log in before you can comment on or make changes to this bug.