Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1303213004:
New illustrations for the Data Reduction Proxy promo (Closed)
|
Created:
5 years, 3 months ago by megjablon Modified:
5 years, 3 months ago Reviewers:
newt (away) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew illustrations for the Data Reduction Proxy promo
Also, fix the button alignment.
BUG=470433, 527266
Committed: https://crrev.com/963891f0ae6078b07aa256c5024a618905c5d287
Cr-Commit-Position: refs/heads/master@{#347798}
Patch Set 1 #
Total comments: 1
Patch Set 2 : image size and buttons fix #
Total comments: 18
Patch Set 3 : match_parent fix #Patch Set 4 : tablet fixes #Patch Set 5 : remove button border #Patch Set 6 : comments #
Messages
Total messages: 26 (3 generated)
megjablon@chromium.org changed reviewers: + newt@chromium.org
New illustrations yay! For some reason the buttons gained a shadow and outline since the last time I built this, do you know what that could be from?
On 2015/09/02 00:10:39, megjablon wrote: > New illustrations yay! Hooray ;) > For some reason the buttons gained a shadow and outline since the last time I > built this, do you know what that could be from? Are you using the same version of Android you were using before? In particular, these buttons will look different on L vs pre-L devices. (Before L, there won't be a shadow). Feel free to send screenshots if you need more input.
https://codereview.chromium.org/1303213004/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right): https://codereview.chromium.org/1303213004/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/data_reduction_promo_screen.xml:50: android:paddingEnd="75dp" Why is this padding needed? And why the max height? We shouldn't be shipping an image that's larger than we're going to display it. We should just check in a smaller image rather than applying maxHeight. (Note: Android automatically adjusts the size of the image depending on the screen density, so the image is always, say, 275dp x 275dp, regardless of whether we're on an mdpi, hdpi, or xxxhdpi device)
On 2015/09/02 00:29:52, newt wrote: > On 2015/09/02 00:10:39, megjablon wrote: > > New illustrations yay! > > Hooray ;) > > > For some reason the buttons gained a shadow and outline since the last time I > > built this, do you know what that could be from? > > Are you using the same version of Android you were using before? In particular, > these buttons will look different on L vs pre-L devices. (Before L, there won't > be a shadow). Feel free to send screenshots if you need more input. I'm using the same device as before which is L, but there wasn't a shadow on it before.
On 2015/09/02 03:31:00, newt wrote: > https://codereview.chromium.org/1303213004/diff/1/chrome/android/java/res/lay... > File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right): > > https://codereview.chromium.org/1303213004/diff/1/chrome/android/java/res/lay... > chrome/android/java/res/layout/data_reduction_promo_screen.xml:50: > android:paddingEnd="75dp" > Why is this padding needed? > > And why the max height? We shouldn't be shipping an image that's larger than > we're going to display it. We should just check in a smaller image rather than > applying maxHeight. (Note: Android automatically adjusts the size of the image > depending on the screen density, so the image is always, say, 275dp x 275dp, > regardless of whether we're on an mdpi, hdpi, or xxxhdpi device) Ah ok. The xxhdpi image was getting too large on my tablet. Should I use a smaller one then?
On 2015/09/02 17:48:06, megjablon wrote: > On 2015/09/02 00:29:52, newt wrote: > > On 2015/09/02 00:10:39, megjablon wrote: > > > New illustrations yay! > > > > Hooray ;) > > > > > For some reason the buttons gained a shadow and outline since the last time > I > > > built this, do you know what that could be from? > > > > Are you using the same version of Android you were using before? In > particular, > > these buttons will look different on L vs pre-L devices. (Before L, there > won't > > be a shadow). Feel free to send screenshots if you need more input. > > I'm using the same device as before which is L, but there wasn't a shadow on it > before. Can you attach screenshots to the bug showing the current state? The screenshots on https://code.google.com/p/chromium/issues/detail?id=527266 ("Data saver dialog buttons are not aligned") look fine to me, so I'm not sure what the issue is.
On 2015/09/02 17:50:39, megjablon wrote: > On 2015/09/02 03:31:00, newt wrote: > > > https://codereview.chromium.org/1303213004/diff/1/chrome/android/java/res/lay... > > File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right): > > > > > https://codereview.chromium.org/1303213004/diff/1/chrome/android/java/res/lay... > > chrome/android/java/res/layout/data_reduction_promo_screen.xml:50: > > android:paddingEnd="75dp" > > Why is this padding needed? > > > > And why the max height? We shouldn't be shipping an image that's larger than > > we're going to display it. We should just check in a smaller image rather than > > applying maxHeight. (Note: Android automatically adjusts the size of the image > > depending on the screen density, so the image is always, say, 275dp x 275dp, > > regardless of whether we're on an mdpi, hdpi, or xxxhdpi device) > > Ah ok. The xxhdpi image was getting too large on my tablet. Should I use a > smaller one then? Is the image is too big (as measured in dp), then we should use a smaller image. I'm surprised the image was only too large on tablet though; I'd expect tablet to have more space to work with.
On 2015/09/02 22:30:29, newt wrote: > On 2015/09/02 17:48:06, megjablon wrote: > > On 2015/09/02 00:29:52, newt wrote: > > > On 2015/09/02 00:10:39, megjablon wrote: > > > > New illustrations yay! > > > > > > Hooray ;) > > > > > > > For some reason the buttons gained a shadow and outline since the last > time > > I > > > > built this, do you know what that could be from? > > > > > > Are you using the same version of Android you were using before? In > > particular, > > > these buttons will look different on L vs pre-L devices. (Before L, there > > won't > > > be a shadow). Feel free to send screenshots if you need more input. > > > > I'm using the same device as before which is L, but there wasn't a shadow on > it > > before. > > Can you attach screenshots to the bug showing the current state? The screenshots > on https://code.google.com/p/chromium/issues/detail?id=527266 ("Data saver > dialog buttons are not aligned") look fine to me, so I'm not sure what the issue > is. Somehow when I was working on the button height fix, this righted itself.
On 2015/09/02 22:31:48, newt wrote: > On 2015/09/02 17:50:39, megjablon wrote: > > On 2015/09/02 03:31:00, newt wrote: > > > > > > https://codereview.chromium.org/1303213004/diff/1/chrome/android/java/res/lay... > > > File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right): > > > > > > > > > https://codereview.chromium.org/1303213004/diff/1/chrome/android/java/res/lay... > > > chrome/android/java/res/layout/data_reduction_promo_screen.xml:50: > > > android:paddingEnd="75dp" > > > Why is this padding needed? > > > > > > And why the max height? We shouldn't be shipping an image that's larger than > > > we're going to display it. We should just check in a smaller image rather > than > > > applying maxHeight. (Note: Android automatically adjusts the size of the > image > > > depending on the screen density, so the image is always, say, 275dp x 275dp, > > > regardless of whether we're on an mdpi, hdpi, or xxxhdpi device) > > > > Ah ok. The xxhdpi image was getting too large on my tablet. Should I use a > > smaller one then? > > Is the image is too big (as measured in dp), then we should use a smaller image. > I'm surprised the image was only too large on tablet though; I'd expect tablet > to have more space to work with. I'm still learning how density-independent pixels work. With no margin set on the image view, by default the image takes up up the whole width of the screen and causes the content to be larger than the height of the frame making the view scroll. On the tablet the image is fuzzy. Does this just mean I need smaller assets?
> I'm still learning how density-independent pixels work. With no margin set on > the image view, by default the image takes up up the whole width of the screen > and causes the content to be larger than the height of the frame making the view > scroll. On the tablet the image is fuzzy. Does this just mean I need smaller > assets? Ah, the problem is that you've set the width of the ImageView to be match_parent. So, Android makes the ImageView as wide as the dialog, then because adjustViewBounds is true, it scales the image up to fill that width. Instead, you should set the width and height of the ImageView to wrap_content and set its layout_gravity to center_horizontal. I think you want to keep adjustViewBounds, though, so that on a tiny device where the dialog is narrower than the image (e.g. less than 260dp), the image will still be scaled down. You should test this though (e.g. by artificially making the dialog very narrow) and make sure that the image reacts appropriately.
On 2015/09/03 01:24:09, newt wrote: > > I'm still learning how density-independent pixels work. With no margin set on > > the image view, by default the image takes up up the whole width of the screen > > and causes the content to be larger than the height of the frame making the > view > > scroll. On the tablet the image is fuzzy. Does this just mean I need smaller > > assets? > > Ah, the problem is that you've set the width of the ImageView to be > match_parent. So, Android makes the ImageView as wide as the dialog, then > because adjustViewBounds is true, it scales the image up to fill that width. > > Instead, you should set the width and height of the ImageView to wrap_content > and set its layout_gravity to center_horizontal. I think you want to keep > adjustViewBounds, though, so that on a tiny device where the dialog is narrower > than the image (e.g. less than 260dp), the image will still be scaled down. You > should test this though (e.g. by artificially making the dialog very narrow) and > make sure that the image reacts appropriately. Yay that fixed it! You're awesome!
Great :) Just have some comments on the button alignment fix. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right): https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:77: <LinearLayout Why the nested LinearLayout? You should be able to remove this. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:94: android:layout_height="wrap_content" This should be match_parent too, in case the first button has longer text.
I have a question about the assets. How does using only the xxhdpi and mdpi densities affect the physical size? On my nexus 7 (which is xhdpi) the resource is very small so it looks like it's using the mdpi. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right): https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:77: <LinearLayout On 2015/09/03 18:07:44, newt wrote: > Why the nested LinearLayout? You should be able to remove this. The first linear layout has to have layout_below so that if the content is larger than the screen height, the buttons don't get pushed over the text. I tried layout_above on the data_reduction_promo_text layout, but the buttons still end up floating over the text. That combined with layout_alignParentBottom causes the layout height to be from the bottom of the text to the bottom of the RelativeLayout. If I don't have the extra LinearLayout the button height ends up being this whole space. With the second linear layout, the buttons end up the appropriate height. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:94: android:layout_height="wrap_content" On 2015/09/03 18:07:44, newt wrote: > This should be match_parent too, in case the first button has longer text. Done.
On 2015/09/03 18:47:53, megjablon wrote: > I have a question about the assets. How does using only the xxhdpi and mdpi > densities affect the physical size? On my nexus 7 (which is xhdpi) the resource > is very small so it looks like it's using the mdpi. > > https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... > File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right): > > https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... > chrome/android/java/res/layout/data_reduction_promo_screen.xml:77: <LinearLayout > On 2015/09/03 18:07:44, newt wrote: > > Why the nested LinearLayout? You should be able to remove this. > > The first linear layout has to have layout_below so that if the content is > larger than the screen height, the buttons don't get pushed over the text. I > tried layout_above on the data_reduction_promo_text layout, but the buttons > still end up floating over the text. That combined with layout_alignParentBottom > causes the layout height to be from the bottom of the text to the bottom of the > RelativeLayout. If I don't have the extra LinearLayout the button height ends up > being this whole space. With the second linear layout, the buttons end up the > appropriate height. > > https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... > chrome/android/java/res/layout/data_reduction_promo_screen.xml:94: > android:layout_height="wrap_content" > On 2015/09/03 18:07:44, newt wrote: > > This should be match_parent too, in case the first button has longer text. > > Done. Nevermind! Disregard that question. I found an answer.
On 2015/09/03 18:47:53, megjablon wrote: > I have a question about the assets. How does using only the xxhdpi and mdpi > densities affect the physical size? On my nexus 7 (which is xhdpi) the resource > is very small so it looks like it's using the mdpi. On ldpi, hdpi, xhdpi, and xxxhdpi devices, Android will scale one of the available image assets down or up to ensure that the image is always displayed in the same size, as measured in "dp". The only visible effect of leaving out certain densities of the image, is that the image will have scaling artifacts on some devices. The size of the image, however, is unchanged, regardless of how many different versions of the image we include in the APK.
(Added comments on the wrong patch set, but they're still applicable) https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right): https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:69: <LinearLayout Change to FrameLayout, which is simpler than LinearLayout. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:74: android:padding="16dp" move the padding and clipToPadding attributes to the inner LinearLayout so that the button shadows don't get clipped. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:76: android:gravity="end|bottom" > remove "end" since it has no effect. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:77: <LinearLayout On 2015/09/03 18:47:53, megjablon wrote: > On 2015/09/03 18:07:44, newt wrote: > > Why the nested LinearLayout? You should be able to remove this. > > The first linear layout has to have layout_below so that if the content is > larger than the screen height, the buttons don't get pushed over the text. I > tried layout_above on the data_reduction_promo_text layout, but the buttons > still end up floating over the text. That combined with layout_alignParentBottom > causes the layout height to be from the bottom of the text to the bottom of the > RelativeLayout. If I don't have the extra LinearLayout the button height ends up > being this whole space. With the second linear layout, the buttons end up the > appropriate height. Ah. Right. I played with this a bit. Seems like what you're doing is the best option. Add a comment though, to ensure that someone doesn't helpfully delete the extra parent layout in an attempt to improve this. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:80: android:gravity="end|bottom" remove "bottom" since it has no effect
Whew! Who knew that getting equal height buttons would be so complicated! https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right): https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:69: <LinearLayout On 2015/09/03 23:11:23, newt wrote: > Change to FrameLayout, which is simpler than LinearLayout. Oddly, gravity="bottom" on the FrameLayout doesn't have an effect on the child LinearLayout. Instead, you need to add layout_gravity="bottom" to the child LinearLayout. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:86: android:layout_marginEnd="8dp" Also add layout_weight="1" to each of the buttons. This will allow the first button to be shrunk in case it has overly long text. (Otherwise, if the first button has long text, it'll take up the almost the entire width, and the second button will be only a single character wide, and many many lines tall.)
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right): https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:69: <LinearLayout On 2015/09/03 23:43:46, newt wrote: > On 2015/09/03 23:11:23, newt wrote: > > Change to FrameLayout, which is simpler than LinearLayout. > > Oddly, gravity="bottom" on the FrameLayout doesn't have an effect on the child > LinearLayout. Instead, you need to add layout_gravity="bottom" to the child > LinearLayout. Last thought: another way to fix this, and perhaps a better user experience, is to move the buttons out of the ScrollView, so they're always visible at the bottom of the dialog. This will ensure that the buttons are visible, even in, e.g. landscape mode on a small phone.
https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/data_reduction_promo_screen.xml (right): https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:69: <LinearLayout On 2015/09/03 23:57:47, newt wrote: > On 2015/09/03 23:43:46, newt wrote: > > On 2015/09/03 23:11:23, newt wrote: > > > Change to FrameLayout, which is simpler than LinearLayout. > > > > Oddly, gravity="bottom" on the FrameLayout doesn't have an effect on the child > > LinearLayout. Instead, you need to add layout_gravity="bottom" to the child > > LinearLayout. > > Last thought: another way to fix this, and perhaps a better user experience, is > to move the buttons out of the ScrollView, so they're always visible at the > bottom of the dialog. This will ensure that the buttons are visible, even in, > e.g. landscape mode on a small phone. For now lets keep it as is. I'd like to get this landed. I can ask Allen if he's ok with that later and switch to that. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:74: android:padding="16dp" On 2015/09/03 23:11:23, newt wrote: > move the padding and clipToPadding attributes to the inner LinearLayout so that > the button shadows don't get clipped. Done. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:76: android:gravity="end|bottom" > On 2015/09/03 23:11:23, newt wrote: > remove "end" since it has no effect. Done. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:77: <LinearLayout On 2015/09/03 23:11:23, newt wrote: > On 2015/09/03 18:47:53, megjablon wrote: > > On 2015/09/03 18:07:44, newt wrote: > > > Why the nested LinearLayout? You should be able to remove this. > > > > The first linear layout has to have layout_below so that if the content is > > larger than the screen height, the buttons don't get pushed over the text. I > > tried layout_above on the data_reduction_promo_text layout, but the buttons > > still end up floating over the text. That combined with > layout_alignParentBottom > > causes the layout height to be from the bottom of the text to the bottom of > the > > RelativeLayout. If I don't have the extra LinearLayout the button height ends > up > > being this whole space. With the second linear layout, the buttons end up the > > appropriate height. > > Ah. Right. I played with this a bit. Seems like what you're doing is the best > option. Add a comment though, to ensure that someone doesn't helpfully delete > the extra parent layout in an attempt to improve this. Done. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:80: android:gravity="end|bottom" On 2015/09/03 23:11:23, newt wrote: > remove "bottom" since it has no effect Done. https://codereview.chromium.org/1303213004/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/data_reduction_promo_screen.xml:86: android:layout_marginEnd="8dp" On 2015/09/03 23:43:46, newt wrote: > Also add layout_weight="1" to each of the buttons. This will allow the first > button to be shrunk in case it has overly long text. (Otherwise, if the first > button has long text, it'll take up the almost the entire width, and the second > button will be only a single character wide, and many many lines tall.) Done.
lgtm
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303213004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303213004/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/963891f0ae6078b07aa256c5024a618905c5d287 Cr-Commit-Position: refs/heads/master@{#347798} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
