Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12737006:
Allow feedback form to be shown in an App Launcher feedback mode. (Closed)
|
Created:
7 years, 9 months ago by benwells Modified:
7 years, 9 months ago CC:
chromium-reviews, arv+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAllow feedback form to be shown in an App Launcher feedback mode.
This mode changes the strings shown, and also hides some parts of the
form such as screenshots and URLs. This mode is opened by showing the
feedback form with a new category tab of "AppLauncher"
BUG=179474
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190138
Patch Set 1 #
Total comments: 1
Patch Set 2 : Woops #
Total comments: 8
Patch Set 3 : First round of feedback #
Total comments: 15
Patch Set 4 : Feedback #Patch Set 5 : Use else instead of continue #
Total comments: 14
Patch Set 6 : Next round #
Total comments: 2
Patch Set 7 : Better CSS #
Total comments: 2
Patch Set 8 : No colspan #Patch Set 9 : Rebase (which removed the category fix) #Patch Set 10 : Touch up rebase #
Messages
Total messages: 25 (0 generated)
tapted for initial review before sending to owners. https://chromiumcodereview.appspot.com/12737006/diff/1/chrome/browser/feedbac... File chrome/browser/feedback/feedback_util.h (right): https://chromiumcodereview.appspot.com/12737006/diff/1/chrome/browser/feedbac... chrome/browser/feedback/feedback_util.h:36: extern const char kAppLauncherCategoryTag[]; This isn't used yet, but will be used from the app launcher when calling ShowFeedbackPage.
Seems lgtm to me with some nits. although a bit unusual to put string constants in the global namespace, but that's a refactor. https://chromiumcodereview.appspot.com/12737006/diff/2001/chrome/app/generate... File chrome/app/generated_resources.grd (right): https://chromiumcodereview.appspot.com/12737006/diff/2001/chrome/app/generate... chrome/app/generated_resources.grd:7928: + Send Feedback. Should dialog titles have a full stop? https://chromiumcodereview.appspot.com/12737006/diff/2001/chrome/app/generate... chrome/app/generated_resources.grd:7930: + <message name="IDS_FEEDBACK_LAUNCHER_DESCRIPTION_LABEL" desc="Label for the description field when asking for launcher feedback"> nit: launcher -> app launcher? https://chromiumcodereview.appspot.com/12737006/diff/2001/chrome/browser/reso... File chrome/browser/resources/feedback.html (right): https://chromiumcodereview.appspot.com/12737006/diff/2001/chrome/browser/reso... chrome/browser/resources/feedback.html:21: <span id="launcher-description" colspan="2" i18n-content="launcher-description" hidden></span> nit: split on to 2 lines https://chromiumcodereview.appspot.com/12737006/diff/2001/chrome/browser/reso... File chrome/browser/resources/feedback.js (right): https://chromiumcodereview.appspot.com/12737006/diff/2001/chrome/browser/reso... chrome/browser/resources/feedback.js:413: // Are screenshots disabled? maybe update this comment (or remove it, since it's pretty much just repeating the if condition)
OK moved into chrome::, much nicer and not a big refactor at all. Just the strings though... https://codereview.chromium.org/12737006/diff/2001/chrome/app/generated_resou... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/12737006/diff/2001/chrome/app/generated_resou... chrome/app/generated_resources.grd:7928: + Send Feedback. On 2013/03/15 05:06:03, tapted wrote: > Should dialog titles have a full stop? In this case, yes. The normal feedback title does, and Jeff mocked the new one with one too. https://codereview.chromium.org/12737006/diff/2001/chrome/app/generated_resou... chrome/app/generated_resources.grd:7930: + <message name="IDS_FEEDBACK_LAUNCHER_DESCRIPTION_LABEL" desc="Label for the description field when asking for launcher feedback"> On 2013/03/15 05:06:03, tapted wrote: > nit: launcher -> app launcher? Done. https://codereview.chromium.org/12737006/diff/2001/chrome/browser/resources/f... File chrome/browser/resources/feedback.html (right): https://codereview.chromium.org/12737006/diff/2001/chrome/browser/resources/f... chrome/browser/resources/feedback.html:21: <span id="launcher-description" colspan="2" i18n-content="launcher-description" hidden></span> On 2013/03/15 05:06:03, tapted wrote: > nit: split on to 2 lines Done. https://codereview.chromium.org/12737006/diff/2001/chrome/browser/resources/f... File chrome/browser/resources/feedback.js (right): https://codereview.chromium.org/12737006/diff/2001/chrome/browser/resources/f... chrome/browser/resources/feedback.js:413: // Are screenshots disabled? On 2013/03/15 05:06:03, tapted wrote: > maybe update this comment (or remove it, since it's pretty much just repeating > the if condition) I've just removed it, doesn't feel that necessary.
+xiyuan for trivial chromeos changes +rkc for chrome/browser/feedback +dbeam for chrome/browser/resources, chrome/browser/ui/webui
https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... File chrome/browser/resources/feedback.html (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... chrome/browser/resources/feedback.html:18: <h1 id="launcher-title" i18n-content="launcher-title" hidden></h1> this should probably be `i18n-content="launcherTitle"` (the [id] is fine) to match current majority of the code... https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... chrome/browser/resources/feedback.html:22: i18n-content="launcher-description" hidden></span> and i18n-content="launcherDescription" https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... File chrome/browser/resources/feedback.js (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... chrome/browser/resources/feedback.js:101: $('page-url').hidden = true; nit: if you want, you can make a class called .launcher-showing and then do something like: .launcher-showing #page-url, .launcher-showing #screenshot-row, .launcher-showing #title, .launcher-showing #description { display: none; } .launcher-showing #launcher-title, .launcher-showing #launcher-description { display: block; } if you want, then here you'd do something like: document.documentElement.classList.add('launcher-showing'); it's kind of a toss up in my head, but we've previously used a single class as a flag to affect many parts of the page https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... File chrome/browser/ui/webui/feedback_ui.cc (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... chrome/browser/ui/webui/feedback_ui.cc:345: source->AddLocalizedString("privacy-note", IDS_FEEDBACK_PRIVACY_NOTE); ^ I guess you guy are already using i18n-keys-with-dashes, so my mention of i18nKeysAsCamelCase isn't a huge deal... it does differ from much of the other webui code, though, IIRC https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... chrome/browser/ui/webui/feedback_ui.cc:447: } what's the difference between if () { continue; } if () { continue; } and if () { } else if () { }?
chrome/browser/chromeos LGTM
https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... File chrome/browser/resources/feedback.html (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... chrome/browser/resources/feedback.html:18: <h1 id="launcher-title" i18n-content="launcher-title" hidden></h1> On 2013/03/15 15:29:51, Dan Beam wrote: > this should probably be `i18n-content="launcherTitle"` (the [id] is fine) to > match current majority of the code... Since the rest of this file follows the word1-word2 notation, it might make sense to leave this as that till someone refactors the whole thing. Otherwise it'll just look out of place. https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... File chrome/browser/resources/feedback.js (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... chrome/browser/resources/feedback.js:101: $('page-url').hidden = true; On 2013/03/15 15:29:51, Dan Beam wrote: > nit: if you want, you can make a class called .launcher-showing and then do > something like: > > .launcher-showing #page-url, > .launcher-showing #screenshot-row, > .launcher-showing #title, > .launcher-showing #description { > display: none; > } > > .launcher-showing #launcher-title, > .launcher-showing #launcher-description { > display: block; > } > > if you want, then here you'd do something like: > > document.documentElement.classList.add('launcher-showing'); > > it's kind of a toss up in my head, but we've previously used a single class as a > flag to affect many parts of the page I'd strongly recommend that we go with the approach Dan outlined. We might have more versions of the feedback page later on, having a class makes it cleaner to implement in JS. We could then have classes like, .normal-feedback .launcher-feedback .settings-feedback etc. https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... File chrome/browser/ui/webui/feedback_ui.cc (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... chrome/browser/ui/webui/feedback_ui.cc:671: , std::string() Might want to actually send the category tag :) Looks like a bug that never got caught since on the feedback backend we really don't do anything with the category tag.
https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... File chrome/browser/resources/feedback.html (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... chrome/browser/resources/feedback.html:18: <h1 id="launcher-title" i18n-content="launcher-title" hidden></h1> On 2013/03/15 16:58:40, Rahul Chaturvedi wrote: > On 2013/03/15 15:29:51, Dan Beam wrote: > > this should probably be `i18n-content="launcherTitle"` (the [id] is fine) to > > match current majority of the code... > Since the rest of this file follows the word1-word2 notation, it might make > sense to leave this as that till someone refactors the whole thing. Otherwise > it'll just look out of place. > yes, it's fine to be locally consistent where it makes sense
https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... File chrome/browser/resources/feedback.html (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... chrome/browser/resources/feedback.html:18: <h1 id="launcher-title" i18n-content="launcher-title" hidden></h1> On 2013/03/15 17:20:42, Dan Beam wrote: > On 2013/03/15 16:58:40, Rahul Chaturvedi wrote: > > On 2013/03/15 15:29:51, Dan Beam wrote: > > > this should probably be `i18n-content="launcherTitle"` (the [id] is fine) to > > > match current majority of the code... > > Since the rest of this file follows the word1-word2 notation, it might make > > sense to leave this as that till someone refactors the whole thing. Otherwise > > it'll just look out of place. > > > > yes, it's fine to be locally consistent where it makes sense OK, left as is for local consistency. https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... File chrome/browser/resources/feedback.js (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/resources/... chrome/browser/resources/feedback.js:101: $('page-url').hidden = true; On 2013/03/15 16:58:40, Rahul Chaturvedi wrote: > On 2013/03/15 15:29:51, Dan Beam wrote: > > nit: if you want, you can make a class called .launcher-showing and then do > > something like: > > > > .launcher-showing #page-url, > > .launcher-showing #screenshot-row, > > .launcher-showing #title, > > .launcher-showing #description { > > display: none; > > } > > > > .launcher-showing #launcher-title, > > .launcher-showing #launcher-description { > > display: block; > > } > > > > if you want, then here you'd do something like: > > > > document.documentElement.classList.add('launcher-showing'); > > > > it's kind of a toss up in my head, but we've previously used a single class as > a > > flag to affect many parts of the page > > I'd strongly recommend that we go with the approach Dan outlined. We might have > more versions of the feedback page later on, having a class makes it cleaner to > implement in JS. > > We could then have classes like, > > .normal-feedback > .launcher-feedback > .settings-feedback > > etc. Done. Note I didn't roll the disabled / enabled screenshot into this. My thinking is this layout control should be a top level thing only. https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... File chrome/browser/ui/webui/feedback_ui.cc (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... chrome/browser/ui/webui/feedback_ui.cc:447: } On 2013/03/15 15:29:51, Dan Beam wrote: > what's the difference between if () { continue; } if () { continue; } and if () > { } else if () { }? I'm not sure there is any difference. TBH I just followed the local pattern, but this way uses more lines. https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... chrome/browser/ui/webui/feedback_ui.cc:671: , std::string() On 2013/03/15 16:58:40, Rahul Chaturvedi wrote: > Might want to actually send the category tag :) > > Looks like a bug that never got caught since on the feedback backend we really > don't do anything with the category tag. Woops, good catch. Done.
https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... File chrome/browser/ui/webui/feedback_ui.cc (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... chrome/browser/ui/webui/feedback_ui.cc:453: continue; also, there's a continue at the very bottom of the loop... that makes no sense to me...
Ben we've discovered an issue with category tags; I've updated the bug with the new information, but please hold off on landing this CL till that issue is resolved.
rkc: Please let me know asap what to do with the category. If required I can prepend it to the description like this: AppLauncher: Stuff the user entered. https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... File chrome/browser/ui/webui/feedback_ui.cc (right): https://codereview.chromium.org/12737006/diff/10001/chrome/browser/ui/webui/f... chrome/browser/ui/webui/feedback_ui.cc:453: continue; On 2013/03/18 06:09:50, Dan Beam wrote: > also, there's a continue at the very bottom of the loop... that makes no sense > to me... It's another form of if / else. The continue at the end is equivalent to having a comma after the last item in a list: it lets someone come later and copy another block to the end, and it just works. I've converted this all to an if / else block.
https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... File chrome/browser/resources/feedback.css (right): https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.css:113: display: block; s/.normal-layout // CSS works by overriding normal/default styles, you don't need to explicitly call them out. https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.css:117: .normal-layout #launcher-description { IMO: this is preferable .launcher-layout #page-url, .launcher-layout #screenshot-row, .launcher-layout #title, .launcher-layout #description, html:not(.launcher-layout) #launcher-title, html:not(.launcher-layout) #launcher-description { display: none; } (assuming you set the class document.documentElement) https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... File chrome/browser/resources/feedback.html (right): https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.html:17: class="normal-layout"> ^ this is unnecessary https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... File chrome/browser/resources/feedback.js (right): https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.js:101: document.body.classList.remove('normal-layout'); ^ I don't think you need this https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.js:102: document.body.classList.add('launcher-layout'); it's arguable that you should be using `document.documentElement` instead https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.js:410: showLauncherFeedback(); this could be reduced to document.documentElement.toggleClass( 'launcher-layout', defaults.launcherFeedback);
Next round. dbeam: thanks for your patience! https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... File chrome/browser/resources/feedback.css (right): https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.css:113: display: block; On 2013/03/19 04:43:53, Dan Beam wrote: > s/.normal-layout // > > CSS works by overriding normal/default styles, you don't need to explicitly call > them out. Done. https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.css:117: .normal-layout #launcher-description { On 2013/03/19 04:43:53, Dan Beam wrote: > IMO: this is preferable > > .launcher-layout #page-url, > .launcher-layout #screenshot-row, > .launcher-layout #title, > .launcher-layout #description, > html:not(.launcher-layout) #launcher-title, > html:not(.launcher-layout) #launcher-description { > display: none; > } > > (assuming you set the class document.documentElement) I am using document now but I find this a bit confusing with the default (was normal-layout) stuff as well. It doesn't feel like it will scale to another type of layout very well. But, my css is weak. Let me know if I should do it with the html:not and I will. https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... File chrome/browser/resources/feedback.html (right): https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.html:17: class="normal-layout"> On 2013/03/19 04:43:53, Dan Beam wrote: > ^ this is unnecessary Cool, done. https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... File chrome/browser/resources/feedback.js (right): https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.js:101: document.body.classList.remove('normal-layout'); On 2013/03/19 04:43:53, Dan Beam wrote: > ^ I don't think you need this Done. https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.js:102: document.body.classList.add('launcher-layout'); On 2013/03/19 04:43:53, Dan Beam wrote: > it's arguable that you should be using `document.documentElement` instead I was using body as I didn't know how to add a style to the document in the html. Now I don't need to :) https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.js:410: showLauncherFeedback(); On 2013/03/19 04:43:53, Dan Beam wrote: > this could be reduced to > > document.documentElement.toggleClass( > 'launcher-layout', defaults.launcherFeedback); That didn't seem to work. Do we have our own toggleClass thing (googling shows it as a jQuery thing).
https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... File chrome/browser/resources/feedback.css (right): https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.css:117: .normal-layout #launcher-description { On 2013/03/20 07:49:32, benwells wrote: > On 2013/03/19 04:43:53, Dan Beam wrote: > > IMO: this is preferable > > > > .launcher-layout #page-url, > > .launcher-layout #screenshot-row, > > .launcher-layout #title, > > .launcher-layout #description, > > html:not(.launcher-layout) #launcher-title, > > html:not(.launcher-layout) #launcher-description { > > display: none; > > } > > > > (assuming you set the class document.documentElement) > > I am using document now but I find this a bit confusing with the default (was > normal-layout) stuff as well. It doesn't feel like it will scale to another type > of layout very well. > > But, my css is weak. Let me know if I should do it with the html:not and I will. grouping these styles together makes it more apparent what changes when the layout flips, i.e. #launcher-title and #launcher-description are hidden when not in launcher layout, #page-url, #screenshot-row, #title, and #description are hidden in launcher layou) https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... File chrome/browser/resources/feedback.js (right): https://codereview.chromium.org/12737006/diff/27018/chrome/browser/resources/... chrome/browser/resources/feedback.js:410: showLauncherFeedback(); On 2013/03/20 07:49:32, benwells wrote: > On 2013/03/19 04:43:53, Dan Beam wrote: > > this could be reduced to > > > > document.documentElement.toggleClass( > > 'launcher-layout', defaults.launcherFeedback); > > That didn't seem to work. Do we have our own toggleClass thing (googling shows > it as a jQuery thing). sorry, meant document.documentElement.classList.toggle() https://codereview.chromium.org/12737006/diff/39001/chrome/browser/resources/... File chrome/browser/resources/feedback.css (right): https://codereview.chromium.org/12737006/diff/39001/chrome/browser/resources/... chrome/browser/resources/feedback.css:113: display: block; this is the default, don't do this. this is akin to: a { border: none; color: blue; line-height: auto; height: auto; text-decoration: underline; width: auto; } (just setting default styles)
ready for next round. rkc: are there more changes you'd like? I won't land until we resolve what to do with category tag... https://codereview.chromium.org/12737006/diff/39001/chrome/browser/resources/... File chrome/browser/resources/feedback.css (right): https://codereview.chromium.org/12737006/diff/39001/chrome/browser/resources/... chrome/browser/resources/feedback.css:113: display: block; On 2013/03/21 01:09:11, Dan Beam wrote: > this is the default, don't do this. this is akin to: > > a { > border: none; > color: blue; > line-height: auto; > height: auto; > text-decoration: underline; > width: auto; > } > > (just setting default styles) OK, got it. Feels much better now.
Meeting with the Feedback PM to get this resolved at 3:30pm Thursday.
lgtm https://chromiumcodereview.appspot.com/12737006/diff/44002/chrome/browser/res... File chrome/browser/resources/feedback.html (right): https://chromiumcodereview.appspot.com/12737006/diff/44002/chrome/browser/res... chrome/browser/resources/feedback.html:21: <span id="launcher-description" colspan="2" er, why is this [colspan] here?
https://chromiumcodereview.appspot.com/12737006/diff/44002/chrome/browser/res... File chrome/browser/resources/feedback.html (right): https://chromiumcodereview.appspot.com/12737006/diff/44002/chrome/browser/res... chrome/browser/resources/feedback.html:21: <span id="launcher-description" colspan="2" On 2013/03/21 02:07:38, Dan Beam wrote: > er, why is this [colspan] here? Good catch, it's now gone here and on the other description line I copied from.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/12737006/58001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/12737006/58001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/12737006/58001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/12737006/58001
Message was sent while issue was closed.
Change committed as 190138 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
