767.92 KB, image/png
1.60 KB, patch
|Details | Diff | Splinter Review|
402.23 KB, image/png
Created attachment 783136 [details] Nightly (07/30) - Screenshot See screenshot. Bug 872388 added 'super-toasts' functionality for bookmarking via our menu. They appear fine on phones but deserve a width constraint on landscape tablets. -- Samsung Galaxy Tab 3 10" (Android 4.2)
Shilpan - Talk to Wes if you need some context on this
Any specifics on how large the width should be?
Created attachment 790965 [details] [diff] [review] Patch for smaller toasts After trying out a lot of different ways of doing this, I finally resorted to have a different width style for toasts on tablets as compared to phones. We can have a larger toast on tablets in my opinion. ~500dp should be safe enough.
Shilpan, mind posting a screenshot?
Comment on attachment 790965 [details] [diff] [review] Patch for smaller toasts Review of attachment 790965 [details] [diff] [review]: ----------------------------------------------------------------- This fixes the width of the toast at 300dp on xlarge screen devices (i.e. it won't stretch to any larger). I just want ian's feedback on that and some comments added to the patch. ::: mobile/android/base/resources/values-large-v11/styles.xml @@ +65,5 @@ > <style name="TabsItemClose"> > <item name="android:nextFocusUp">@+id/info</item> > </style> > > + <style name="Toast"> Sorry for the delay here. I wish we didn't have to override everything for this to work. Maybe just a way to make it more obvious what's different for this style. I think the right way to do this is probably to split out shared theme parts and try to only override what's necessary here. That probably means this needs to be something like <style name="Toast"> ... shared stuff </style> <style name="Toast.DeviceSpecific"> ... device specific stuff? </style> That feels like overkill I think though, so maybe we're better just commenting this extensively so that its obvious what's different here. i.e. even something like: <style name="Toast"> ... devices specific stuff... // If you change anything below here, also update it in values/styles.xml .. shared stuff...
We need to push this in 24 if at all possible. Let's get past feedback and into review.
(In reply to Wesley Johnston (:wesj) from comment #9) > This fixes the width of the toast at 300dp on xlarge screen devices (i.e. it > won't stretch to any larger). I just want ian's feedback on that and some > comments added to the patch. Can I see a screenshot of that?
Shilpan showed me a screenshot in IRC, I think the fixed size is fine.
This doesn't have r+ yet AFAICT.
r+ from me and landed: https://hg.mozilla.org/integration/fx-team/rev/6366f575b4c6
Comment on attachment 790965 [details] [diff] [review] Patch for smaller toasts [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 872388 User impact if declined: This one particular toast is pretty ugly Testing completed (on m-c, etc.): Landed on mc today Risk to taking this patch (and alternatives if risky): very low risk. Just a style change for tablets. String or IDL/UUID changes made by this patch: none.
Comment on attachment 790965 [details] [diff] [review] Patch for smaller toasts Approving as this is a low risk cosmetic change.Pease make sure to land by Thursday morning PT so it get into our second last mobile beta.
Verified fixed on: Build: Firefox for Android 26.0a1 (2013-09-05) and Firefox for Android 25.0a2 (2013-09-05) Device: Asus Transformer TF 101 OS: Android 4.0.4
Verified fixed on Firefox 24 Beta 8