Closed Bug 1246239 Opened 8 years ago Closed 8 years ago

Create snackbar feedback when user bookmarks a RV item

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox48 verified)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- verified

People

(Reporter: antlam, Assigned: ahunt)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached image prev_snackbar_savedoffline2.png (obsolete) —
Bug 1246238 handles the "first time" special case but, we should also reinforce the message thereafter with our usual snack bars. 

I've included the offline icon in the snackbar and made these blue to continue driving home the connection of "offline" and "blue" in our users heads. 

For example, bug 1233250 uses the same blue snack bar to indicate the page is offline too.

NOT FIRST TIME:
1) User presses "Reader View" icon in URL bar
2) User presses Bookmark icon in menu
3) Display snackbar

Note: "EDIT" might not be in scope here, feel free to skip that part.
^Covering the Reader View button underneath isn't great [1] but I'll see what :ahunt says in bug 1226238. I don't think its a major issue though since this does dismiss. If anything, it will be design changes to the big orange button in bug 1226238, not this snack bar.

Note: Copy TBD

[1]https://www.google.com/design/spec/components/snackbars-toasts.html#snackbars-toasts-usage
Copy finalized.
Attachment #8716421 - Attachment is obsolete: true
This is mostly implemented, but :sebastian, :margaret and I had some concerns about the icon in the snackbar. It's technically doable, but not something that's officially supported (so it could easily break in future), and it looks like the Android design guidelines tel us not use icons [1]. Do we still want to put in the icon?

[1] Under "Very short text strings" in https://www.google.com/design/spec/components/snackbars-toasts.html#
Assignee: nobody → ahunt
Status: NEW → ASSIGNED
(In reply to Andrzej Hunt :ahunt from comment #4)
> This is mostly implemented, but :sebastian, :margaret and I had some
> concerns about the icon in the snackbar. It's technically doable, but not
> something that's officially supported (so it could easily break in future),
> and it looks like the Android design guidelines tel us not use icons [1]. Do
> we still want to put in the icon?
> 
> [1] Under "Very short text strings" in
> https://www.google.com/design/spec/components/snackbars-toasts.html#

Since antlam is PTO, let's leave out the icon for now and file a follow-up bug to hash this out.

If this isn't part of the design guidelines, and it's something that can break, I don't think it's worth doing it.
I think the visual connection of the icon makes this connection much stronger. While the UX won't break without it, I talked to :ahunt and it doesn't sound like it's too hard either.

For devices/versions that we know this breaks on, we don't have to show the icon. But for those that can benefit from it, should.
Comment on attachment 8733914 [details]
MozReview Request: Bug 1246239 - Show "saved offline" snackbar when readermode page is bookmarked r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42017/diff/1-2/
Attachment #8733914 - Attachment description: MozReview Request: WIP: Bug 1246239 - Show "saved offline" snackbar when readermode page is bookmarked → MozReview Request: Bug 1246239 - Show "saved offline" snackbar when readermode page is bookmarked r?sebastian
Attachment #8733914 - Flags: review?(s.kaspari)
Comment on attachment 8733914 [details]
MozReview Request: Bug 1246239 - Show "saved offline" snackbar when readermode page is bookmarked r?sebastian

https://reviewboard.mozilla.org/r/42017/#review42571

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:455
(Diff revision 2)
>                  getResources().getString(R.string.bookmark_options),
>                  callback);
>      }
>  
> +    private void showReaderModeBookmarkAddedSnackbar() {
> +        final Drawable iconDownloaded = DrawableUtil.tintDrawable(getContext(), R.drawable.status_icon_readercache, R.color.saved_offline_icon_color);

Did you create the color resource to use tintDrawable? Let's make DrawableUtil work with actual colors too. I don't think "saved_offline_icon_color" is something we'll use more often in our code base.

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:108
(Diff revision 2)
> +    public static void showSnackbarWithActionAndColors(Activity activity,
> +                                                       String message,
> +                                                       int duration,
> +                                                       String action,
> +                                                       SnackbarCallback callback,
> +                                                       Drawable icon,
> +                                                       Integer backgroundColor,
> +                                                       Integer actionColor) {

I'm not too happy about this parameter explosion (and the Autoboxing that comes with it). Maybe we should transform this in something that uses the builder pattern in a follow-up bug. There are a bunch of showSnackbar*() methods now.

::: mobile/android/base/java/org/mozilla/gecko/SnackbarHelper.java:130
(Diff revision 2)
> +            final InsetDrawable paddedIcon = new InsetDrawable(icon, 0, 0, (int) activity.getResources().getDisplayMetrics().density *  10, 0);
> +
> +            paddedIcon.setBounds(0, 0, ((int) activity.getResources().getDisplayMetrics().density * 10) + icon.getIntrinsicWidth(), icon.getIntrinsicHeight());

I usually prefer to use TypedValue to convert from dp to pixels. It's more descriptive:
TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, 10, displayMetrics).

Maybe let's do the calculation only once here.

NIT: If you keep this: There are two spaces before the first "* 10". :)

::: mobile/android/base/locales/en-US/android_strings.dtd:75
(Diff revision 2)
>  <!ENTITY screenshot_folder_label_in_bookmarks "Screenshots">
>  <!ENTITY readinglist_smartfolder_label_in_bookmarks "Reading List">
>  
> +<!ENTITY reader_saved_offline "Saved offline">
> +<!-- Localization note (reader_switch_to_bookmarks) : This
> +     string is used as an action in a toast - it lets you

In a toast?
Attachment #8733914 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/integration/fx-team/rev/55f774304456fba311e048824adf6df207c5d760
Bug 1246239 - Show "saved offline" snackbar when readermode page is bookmarked r=sebastian
https://reviewboard.mozilla.org/r/42017/#review42571

> I'm not too happy about this parameter explosion (and the Autoboxing that comes with it). Maybe we should transform this in something that uses the builder pattern in a follow-up bug. There are a bunch of showSnackbar*() methods now.

Filed as Bug 1264791
Blocks: 1264791
https://hg.mozilla.org/mozilla-central/rev/55f774304456
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Can someone give a bit of background about this feature? I've been staring at the strings and screenshot for a while, and can't figure them out: if I understand this, we're showing the bar to users bookmarking an article in reader view. "Switch" is meant to open the bookmarks panel, so I guess "See"/"View" are suitable alternatives. Why are we introducing the concept of "Offline"? Is it important to keep it, instead of just saying "Saved"?
One more note based on the screenshot: are you using CSS or string transformation to render that SWITCH? If that's the case, you really shouldn't do that, and provide a "SWITCH" string instead.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#CSS_issues
Bookmarking a page while in reader view, will display a snackbar at the bottom of the screen: "Saved offline | SWITCH". Tapping it, will display the Bookmarks panel.
Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 48.0a1 (2016-04-17)
(In reply to Francesco Lodolo [:flod] from comment #13)
> Can someone give a bit of background about this feature? I've been staring
> at the strings and screenshot for a while, and can't figure them out: if I
> understand this, we're showing the bar to users bookmarking an article in
> reader view. "Switch" is meant to open the bookmarks panel, so I guess
> "See"/"View" are suitable alternatives. Why are we introducing the concept
> of "Offline"? Is it important to keep it, instead of just saying "Saved"?

The key distinction is that bookmarked reader-view articles are saved in an offline "cache", i.e. the content can still be accessed without connectivity (whereas with a normal bookmark you'll see the network error page if you load it without connectivity).

(In reply to Francesco Lodolo [:flod] from comment #14)
> One more note based on the screenshot: are you using CSS or string
> transformation to render that SWITCH? If that's the case, you really
> shouldn't do that, and provide a "SWITCH" string instead.
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/
> Localization_content_best_practices#CSS_issues

Android does it's own String conversion for Snackbars, although I can't find where they actually do this in the sources. I suspect it's just via an "app:allCaps=true" attribute being set somewhere (which we can't change, since it's Android framework code). Is this likely to be an issue for us?
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.