Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10382142:
whip the feedback page into shape (Closed)
|
Created:
8 years, 7 months ago by Evan Stade Modified:
8 years, 7 months ago Reviewers:
Dan Beam CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionwhip the feedback page into shape
this is just a refactor with some minor visual fixes. I will follow this up with an additional CL that brings the page into pixel-perfect alignment with the mock.
1. don't use tables for layout
2. don't copy-paste CSS all over the place
3. delete a bunch of CSS that had no effect or purpose
4. don't flicker when switching between current screenshot and saved screenshot (chromeos)
BUG=125621
TEST=manual (on chromeos)
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137994
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : upload #
Total comments: 27
Patch Set 4 : nits #Patch Set 5 : wrap #Patch Set 6 : sync #
Messages
Total messages: 10 (0 generated)
I know there are still issues with the feedback page after this CL, but it's impossible to fix them all in one go. Therefore please refrain from pointing out further style fixes in existing code unless there's a good reason to make them now.
ping
http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.css (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.css:64: .image-thumbnail-container:not(:only-of-type):not(.image-thumbnail-container-selected):hover { 80 char wrap http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.html (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.html:25: <span id="page-url-label" unwrap this all the tons of tags below that no longer require this http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.html:31: <!-- User e-mail --> there's an extra space after "<!--" in all these comments http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.html:66: <a id="screenshot-link-tosaved" href="#" why not -to-saved and -to-current? http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.html:70: i18n-content="choose-original-screenshot" shouldn't these be 4\s wrapped? http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.js (left): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:61: if (divId == 'current-screenshots') so are you handling this somewhere else or was this functionality deleted? http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.js (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:33: thumbnailDivs[i].className = 'image-thumbnail-container'; you're setting [className] to intentionally nuke the previous (possibly selected) class, right? does this have any artifacts when the same image is selected if we use transitions? http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:37: if ((thumbnailDivs[i].id == thumbnailId) || (!thumbnailId && !i)) { remove first set of () http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:38: thumbnailDivs[i].classList.add('image-thumbnail-container-selected'); I assume you're against simply making this .selected so you could say: .image-thumbnail-container.selected:not(:only-of-type) { border-color: green; } as the class is too generic (and could collide). Or simply changed to [selected] like we do in shared lists (IIRC)?
http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.css (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.css:64: .image-thumbnail-container:not(:only-of-type):not(.image-thumbnail-container-selected):hover { On 2012/05/16 22:38:14, Dan Beam wrote: > 80 char wrap how do you wrap a css selector? http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.html (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.html:25: <span id="page-url-label" On 2012/05/16 22:38:14, Dan Beam wrote: > unwrap this all the tons of tags below that no longer require this Done. http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.html:31: <!-- User e-mail --> On 2012/05/16 22:38:14, Dan Beam wrote: > there's an extra space after "<!--" in all these comments Done. http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.html:66: <a id="screenshot-link-tosaved" href="#" On 2012/05/16 22:38:14, Dan Beam wrote: > why not -to-saved and -to-current? ask the original author http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.html:70: i18n-content="choose-original-screenshot" On 2012/05/16 22:38:14, Dan Beam wrote: > shouldn't these be 4\s wrapped? Done. http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.js (left): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:61: if (divId == 'current-screenshots') On 2012/05/16 22:38:14, Dan Beam wrote: > so are you handling this somewhere else or was this functionality deleted? i don't think this really did anything useful and you can see that I deleted the CSS which referenced image-thumbnail-current http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.js (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:33: thumbnailDivs[i].className = 'image-thumbnail-container'; On 2012/05/16 22:38:14, Dan Beam wrote: > you're setting [className] to intentionally nuke the previous (possibly > selected) class, right? yes > does this have any artifacts when the same image is > selected if we use transitions? i doubt it, but we don't use transitions. http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:37: if ((thumbnailDivs[i].id == thumbnailId) || (!thumbnailId && !i)) { On 2012/05/16 22:38:14, Dan Beam wrote: > remove first set of () Done. http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:38: thumbnailDivs[i].classList.add('image-thumbnail-container-selected'); On 2012/05/16 22:38:14, Dan Beam wrote: > I assume you're against simply making this .selected so you could say: > > .image-thumbnail-container.selected:not(:only-of-type) { > border-color: green; > } > > as the class is too generic (and could collide). Or simply changed to [selected] > like we do in shared lists (IIRC)? yes
http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.css (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.css:64: .image-thumbnail-container:not(:only-of-type):not(.image-thumbnail-container-selected):hover { On 2012/05/16 23:56:24, Evan Stade wrote: > On 2012/05/16 22:38:14, Dan Beam wrote: > > 80 char wrap > > how do you wrap a css selector? either as you would a function or before a psuedo-function, i.e: .image-thumbnail-container:not(:only-of-type):not( .image-thumbnail-container-selected):hover { border-color: rgb(184, 218, 176); } or .image-thumbnail-container:not(:only-of-type) :not(.image-thumbnail-container-selected):hover { border-color: rgb(184, 218, 176); } here's how I've seen it previously (but there was a space to wrap more naturally): https://chromiumcodereview.appspot.com/10332049/diff/11004/chrome/browser/res... (L416) http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.html (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.html:66: <a id="screenshot-link-tosaved" href="#" On 2012/05/16 23:56:24, Evan Stade wrote: > On 2012/05/16 22:38:14, Dan Beam wrote: > > why not -to-saved and -to-current? > > ask the original author I thought I was asking the current owner, ;) http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.js (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:38: thumbnailDivs[i].classList.add('image-thumbnail-container-selected'); On 2012/05/16 23:56:24, Evan Stade wrote: > On 2012/05/16 22:38:14, Dan Beam wrote: > > I assume you're against simply making this .selected so you could say: > > > > .image-thumbnail-container.selected:not(:only-of-type) { > > border-color: green; > > } > > > > as the class is too generic (and could collide). Or simply changed to > [selected] > > like we do in shared lists (IIRC)? > > yes what about second option - [selected]? you're against both? (I understand it was a poorly worded question)
http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.css (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.css:64: .image-thumbnail-container:not(:only-of-type):not(.image-thumbnail-container-selected):hover { On 2012/05/17 05:09:05, Dan Beam wrote: > On 2012/05/16 23:56:24, Evan Stade wrote: > > On 2012/05/16 22:38:14, Dan Beam wrote: > > > 80 char wrap > > > > how do you wrap a css selector? > > either as you would a function or before a psuedo-function, i.e: > > .image-thumbnail-container:not(:only-of-type):not( > .image-thumbnail-container-selected):hover { > border-color: rgb(184, 218, 176); > } > > or > > .image-thumbnail-container:not(:only-of-type) > :not(.image-thumbnail-container-selected):hover { > border-color: rgb(184, 218, 176); > } > > here's how I've seen it previously (but there was a space to wrap more > naturally): > https://chromiumcodereview.appspot.com/10332049/diff/11004/chrome/browser/res... > (L416) that only worked because there was already whitespace in the selector: this selector has no whitespace and adding a newline would change the meaning. http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.html (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.html:66: <a id="screenshot-link-tosaved" href="#" On 2012/05/17 05:09:05, Dan Beam wrote: > On 2012/05/16 23:56:24, Evan Stade wrote: > > On 2012/05/16 22:38:14, Dan Beam wrote: > > > why not -to-saved and -to-current? > > > > ask the original author > > I thought I was asking the current owner, ;) false, if I "owned" everything I'd ever touched I would be a much wealthier man http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.js (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:38: thumbnailDivs[i].classList.add('image-thumbnail-container-selected'); On 2012/05/17 05:09:05, Dan Beam wrote: > On 2012/05/16 23:56:24, Evan Stade wrote: > > On 2012/05/16 22:38:14, Dan Beam wrote: > > > I assume you're against simply making this .selected so you could say: > > > > > > .image-thumbnail-container.selected:not(:only-of-type) { > > > border-color: green; > > > } > > > > > > as the class is too generic (and could collide). Or simply changed to > > [selected] > > > like we do in shared lists (IIRC)? > > > > yes > > what about second option - [selected]? you're against both? (I understand it was > a poorly worded question) I don't see how selected as an attribute rather than class gets around the problem. I don't see much of a drawback to having this long classname except that it causes an unfortunate line wrap which is really not the end of the world.
http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.css (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.css:64: .image-thumbnail-container:not(:only-of-type):not(.image-thumbnail-container-selected):hover { On 2012/05/17 16:38:57, Evan Stade wrote: > On 2012/05/17 05:09:05, Dan Beam wrote: > > On 2012/05/16 23:56:24, Evan Stade wrote: > > > On 2012/05/16 22:38:14, Dan Beam wrote: > > > > 80 char wrap > > > > > > how do you wrap a css selector? > > > > either as you would a function or before a psuedo-function, i.e: > > > > .image-thumbnail-container:not(:only-of-type):not( > > .image-thumbnail-container-selected):hover { > > border-color: rgb(184, 218, 176); > > } > > > > or > > > > .image-thumbnail-container:not(:only-of-type) > > :not(.image-thumbnail-container-selected):hover { > > border-color: rgb(184, 218, 176); > > } > > > > here's how I've seen it previously (but there was a space to wrap more > > naturally): > > > https://chromiumcodereview.appspot.com/10332049/diff/11004/chrome/browser/res... > > (L416) > > that only worked because there was already whitespace in the selector: this > selector has no whitespace and adding a newline would change the meaning. wrapping like C++/JS functions (see #1) doesn't change anything, second one might (didn't check) http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.js (left): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:61: if (divId == 'current-screenshots') On 2012/05/16 23:56:24, Evan Stade wrote: > On 2012/05/16 22:38:14, Dan Beam wrote: > > so are you handling this somewhere else or was this functionality deleted? > > i don't think this really did anything useful and you can see that I deleted the > CSS which referenced image-thumbnail-current OK. http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... File chrome/browser/resources/feedback.js (right): http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... chrome/browser/resources/feedback.js:38: thumbnailDivs[i].classList.add('image-thumbnail-container-selected'); On 2012/05/17 16:38:57, Evan Stade wrote: > On 2012/05/17 05:09:05, Dan Beam wrote: > > On 2012/05/16 23:56:24, Evan Stade wrote: > > > On 2012/05/16 22:38:14, Dan Beam wrote: > > > > I assume you're against simply making this .selected so you could say: > > > > > > > > .image-thumbnail-container.selected:not(:only-of-type) { > > > > border-color: green; > > > > } > > > > > > > > as the class is too generic (and could collide). Or simply changed to > > > [selected] > > > > like we do in shared lists (IIRC)? > > > > > > yes > > > > what about second option - [selected]? you're against both? (I understand it > was > > a poorly worded question) > > I don't see how selected as an attribute rather than class gets around the > problem. I don't see much of a drawback to having this long classname except > that it causes an unfortunate line wrap which is really not the end of the > world. I think it's just less common for somebody to have a generic [selected] selector, but is does have a similar issue (but again, I feel like we should just be getting rid of this problem with namespacing, i.e. .module node[selected] vs. something as silly as just [selected]). If you're OK with the really long class name, that's fine.
On 2012/05/17 17:14:37, Dan Beam wrote: > http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... > File chrome/browser/resources/feedback.css (right): > > http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... > chrome/browser/resources/feedback.css:64: > .image-thumbnail-container:not(:only-of-type):not(.image-thumbnail-container-selected):hover > { > On 2012/05/17 16:38:57, Evan Stade wrote: > > On 2012/05/17 05:09:05, Dan Beam wrote: > > > On 2012/05/16 23:56:24, Evan Stade wrote: > > > > On 2012/05/16 22:38:14, Dan Beam wrote: > > > > > 80 char wrap > > > > > > > > how do you wrap a css selector? > > > > > > either as you would a function or before a psuedo-function, i.e: > > > > > > .image-thumbnail-container:not(:only-of-type):not( > > > .image-thumbnail-container-selected):hover { > > > border-color: rgb(184, 218, 176); > > > } > > > > > > or > > > > > > .image-thumbnail-container:not(:only-of-type) > > > :not(.image-thumbnail-container-selected):hover { > > > border-color: rgb(184, 218, 176); > > > } > > > > > > here's how I've seen it previously (but there was a space to wrap more > > > naturally): > > > > > > https://chromiumcodereview.appspot.com/10332049/diff/11004/chrome/browser/res... > > > (L416) > > > > that only worked because there was already whitespace in the selector: this > > selector has no whitespace and adding a newline would change the meaning. > > wrapping like C++/JS functions (see #1) doesn't change anything, second one > might (didn't check) yes, although the second example obviously does not work, the first example does work in chrome. I was worried it's not allowed in the spec but apparently: Whitespace is permitted after the "(", before the ")", [...] this still seems like a cop-out because we're saved by the fact there's a pseudo-selector with a parenthetical. But fine, I'll change it. In the future I shall endeavor to craft a long selector that has no parentheses anywhere. > > http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... > File chrome/browser/resources/feedback.js (left): > > http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... > chrome/browser/resources/feedback.js:61: if (divId == 'current-screenshots') > On 2012/05/16 23:56:24, Evan Stade wrote: > > On 2012/05/16 22:38:14, Dan Beam wrote: > > > so are you handling this somewhere else or was this functionality deleted? > > > > i don't think this really did anything useful and you can see that I deleted > the > > CSS which referenced image-thumbnail-current > > OK. > > http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... > File chrome/browser/resources/feedback.js (right): > > http://codereview.chromium.org/10382142/diff/4001/chrome/browser/resources/fe... > chrome/browser/resources/feedback.js:38: > thumbnailDivs[i].classList.add('image-thumbnail-container-selected'); > On 2012/05/17 16:38:57, Evan Stade wrote: > > On 2012/05/17 05:09:05, Dan Beam wrote: > > > On 2012/05/16 23:56:24, Evan Stade wrote: > > > > On 2012/05/16 22:38:14, Dan Beam wrote: > > > > > I assume you're against simply making this .selected so you could say: > > > > > > > > > > .image-thumbnail-container.selected:not(:only-of-type) { > > > > > border-color: green; > > > > > } > > > > > > > > > > as the class is too generic (and could collide). Or simply changed to > > > > [selected] > > > > > like we do in shared lists (IIRC)? > > > > > > > > yes > > > > > > what about second option - [selected]? you're against both? (I understand it > > was > > > a poorly worded question) > > > > I don't see how selected as an attribute rather than class gets around the > > problem. I don't see much of a drawback to having this long classname except > > that it causes an unfortunate line wrap which is really not the end of the > > world. > > I think it's just less common for somebody to have a generic [selected] > selector, but is does have a similar issue (but again, I feel like we should > just be getting rid of this problem with namespacing, i.e. .module > node[selected] vs. something as silly as just [selected]). > > If you're OK with the really long class name, that's fine.
ping
lgtm |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
