Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7291004:
Wire up notifications to the New Tab page. (Closed)
|
Created:
9 years, 5 months ago by Finnur Modified:
9 years, 5 months ago CC:
chromium-reviews, estade+watch_chromium.org, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionWire up notifications to the New Tab page.
BUG=88067
TEST=asargent has an extension to test this with.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91420
Patch Set 1 #
Total comments: 37
Patch Set 2 : '' #
Total comments: 2
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 1
Messages
Total messages: 8 (0 generated)
This is a NTP prototype for App Notifications, a continuation of this changelist: http://codereview.chromium.org/7187023/, which introduced the concept of an Experimental App Notification API. This is at a stage where this is working and people (Glen et al) want to start experimenting with it. To get notifications to appear you'd need to enable experimental APIs and have an extension that sends a notification. I'll add a screenshot to the bug so you know what it looks like. Not updating the NTP4 or whatever it is called, since this is in progress still and not finalized (checking in so people can experiment further).
http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... File chrome/browser/resources/new_tab.html (right): http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... chrome/browser/resources/new_tab.html:156: class="app_notification_close" /> indent is 5, should be 4. Don't need / before >. Classes and IDs should use hyphens rather than underscores. http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... chrome/browser/resources/new_tab.html:157: <strong id="notification_title">Title</strong><br> Do you really want this text content? http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... chrome/browser/resources/new_tab.html:161: <div id="arrow_border" class="arrow arrow_border"></div> I can't really say without seeing the final product, but it seems like there should only be a single arrow div, and its appearance (border, shadow, etc.) should be controlled with css http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... chrome/browser/resources/new_tab.html:162: <div id="arrow_shadow" class="arrow arrow_shadow"></div> why do you have arrow_shadow as both an id and class? http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... File chrome/browser/resources/ntp/apps.css (right): http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:102: color: Gray; don't upper case the color http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:111: width: 14px; alphabetize http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:222: width: 200px; alphabetize the properties it is crappy that related properties such as width and height end up far from each other, but it makes it easier to find whether a property is defined (don't have to search through the whole list of properties) http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:224: min-height: 100px; where do you set height? http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:225: min-width: 200px; where do you set width (besides above)? http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:231: -webkit-box-shadow: 0px 0px 2px 1px rgba(0, 0, 0, 0.2); write 0 instead of 0px http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:236: padding: 10px 10px 10px 10px; padding: 10px suffices http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... File chrome/browser/resources/ntp/apps.js (right): http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:174: document.getElementById('notification_title').textContent = title; $() instead of document.getElementById http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:179: var target = event.target; event.currentTarget will do what you want http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:197: x = x - arrow.style.width - 23; -= ? http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:208: bubble.style.opacity = 1; better to do this with a css class than manually editing css in js http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:821: var notif_div = document.createElement('div') 1. all variable names should be camel case 2. I don't understand where notif_div is used. You have a single notification bubble for all apps, right? So where does notif_div fit in (please answer via in-code comments)
All but one comment addressed/replied to. Sending out for hopefully one more round of comments before you go home (given different time zones and that time is of the essence for me). :) http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... File chrome/browser/resources/new_tab.html (right): http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... chrome/browser/resources/new_tab.html:156: class="app_notification_close" /> On 2011/06/30 23:20:33, Evan Stade wrote: > indent is 5, should be 4. Don't need / before >. Classes and IDs should use > hyphens rather than underscores. Done. http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... chrome/browser/resources/new_tab.html:157: <strong id="notification_title">Title</strong><br> On 2011/06/30 23:20:33, Evan Stade wrote: > Do you really want this text content? Done. http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... chrome/browser/resources/new_tab.html:161: <div id="arrow_border" class="arrow arrow_border"></div> See screenshot in bug. In principal, I agree and I tried but I can't find any way to create a single div that's triangular in shape and have borders and shadows work properly (borders and shadows appear rectangular). In fact, this is the way people recommend you do it because it is hard (impossible?) otherwise. If you have tricks up your sleeve, I'd be willing to consider them for a future changelist (I'm running out of time due to vacation that is coming up). :) On 2011/06/30 23:20:33, Evan Stade wrote: > I can't really say without seeing the final product, but it seems like there > should only be a single arrow div, and its appearance (border, shadow, etc.) > should be controlled with css http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... chrome/browser/resources/new_tab.html:162: <div id="arrow_shadow" class="arrow arrow_shadow"></div> On 2011/06/30 23:20:33, Evan Stade wrote: > why do you have arrow_shadow as both an id and class? The class is for .css referencing and the id is to be able to do GetElementById('arrow_border'). GetElementByClassName('arrow_border') doesn't work, probably because the classes are 'arrow arrow_border' not 'arrow_border', and relying on the class names and their order not changing seems brittle. http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... File chrome/browser/resources/ntp/apps.css (right): http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:102: color: Gray; On 2011/06/30 23:20:33, Evan Stade wrote: > don't upper case the color Done. http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:111: width: 14px; On 2011/06/30 23:20:33, Evan Stade wrote: > alphabetize Done. http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:222: width: 200px; On 2011/06/30 23:20:33, Evan Stade wrote: > alphabetize the properties > > it is crappy that related properties such as width and height end up far from > each other, but it makes it easier to find whether a property is defined (don't > have to search through the whole list of properties) Done. http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:224: min-height: 100px; I don't. This is set by the content at the moment. On 2011/06/30 23:20:33, Evan Stade wrote: > where do you set height? http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:225: min-width: 200px; I don't follow. Why do I need to set it elsewhere (besides above)? The bubble has a fixed width of 200px and grows down based on text content. On 2011/06/30 23:20:33, Evan Stade wrote: > where do you set width (besides above)? http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:231: -webkit-box-shadow: 0px 0px 2px 1px rgba(0, 0, 0, 0.2); On 2011/06/30 23:20:33, Evan Stade wrote: > write 0 instead of 0px Done. http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:236: padding: 10px 10px 10px 10px; On 2011/06/30 23:20:33, Evan Stade wrote: > padding: 10px suffices Done. http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... File chrome/browser/resources/ntp/apps.js (right): http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:174: document.getElementById('notification_title').textContent = title; On 2011/06/30 23:20:33, Evan Stade wrote: > $() instead of document.getElementById Done. http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:179: var target = event.target; Not sure what you mean here. event.target and event.currentTarget point to the same element: the span that is the child of the <a> tag. I need the <a> tag here. On 2011/06/30 23:20:33, Evan Stade wrote: > event.currentTarget will do what you want http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:197: x = x - arrow.style.width - 23; On 2011/06/30 23:20:33, Evan Stade wrote: > -= ? Done. http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:208: bubble.style.opacity = 1; I always find it a little silly to create a class around one css property and start flipping classes around instead of just changing the property. Seems like the extra level of indirection doesn't result in simpler code. But, if you feel strongly about this I can take a look in the morning (it is past midnight here). On 2011/06/30 23:20:33, Evan Stade wrote: > better to do this with a css class than manually editing css in js http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:821: var notif_div = document.createElement('div') This isn't about the bubble. It is about the text under the icon that triggers the bubble. I've changed the name and added comments to explain. On 2011/06/30 23:20:33, Evan Stade wrote: > 1. all variable names should be camel case > > 2. I don't understand where notif_div is used. You have a single notification > bubble for all apps, right? So where does notif_div fit in (please answer via > in-code comments)
http://codereview.chromium.org/7291004/diff/4002/chrome/browser/resources/ntp... File chrome/browser/resources/ntp/apps.js (right): http://codereview.chromium.org/7291004/diff/4002/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:818: if (app.notifications && app.notifications.length > 0) { FYI, in the code I ended up checking in, I just put the most recent notification in a property called "notification" instead of the full list in a "notifications" property, so you'll need to fix that here. See: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/ntp/a... I pulled in your patch from this codereview, made the edit I mention above, but the UI doesn't seem to be working correctly for me, so you might want to update to trunk and test things yourself.
Evan, I addressed the last comment remaining. I had the need to add another property, so encapsulating this in a style and flipping the style now makes more sense. Please take another look. Antony, please try again with this changelist. http://codereview.chromium.org/7291004/diff/4002/chrome/browser/resources/ntp... File chrome/browser/resources/ntp/apps.js (right): http://codereview.chromium.org/7291004/diff/4002/chrome/browser/resources/ntp... chrome/browser/resources/ntp/apps.js:818: if (app.notifications && app.notifications.length > 0) { > I just put the most recent notification in a property > called "notification" instead of ... I know. This is already wired up in this changelist. > so you might want to update to trunk and > test things yourself. I did just now, and it works, except that you have to press F5 to get the new notifications to show up on the NTP (the changelist I was working against recreated the apps when new notifications came in, which is something that changed with your last changelist). I have now changed it so that updates request new app info for all apps, which should be enough for the prototype. On 2011/07/01 05:03:06, Antony Sargent wrote: > FYI, in the code I ended up checking in, I just put the most recent notification > in a property called "notification" instead of the full list in a > "notifications" property, so you'll need to fix that here. See: > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/webui/ntp/a... > > I pulled in your patch from this codereview, made the edit I mention above, but > the UI doesn't seem to be working correctly for me, so you might want to update > to trunk and test things yourself.
> Antony, please try again with this changelist. It seemed to work - so I must have a bug in some uncommitted changes I have in my local tree (to support an extension setting notifictions for other extensions). Sorry for the false alarm.
having seen the arrow, it seems like you could create a square div with border and shadow and apply -webkit-transform: rotate(45deg) http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... File chrome/browser/resources/new_tab.html (right): http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/new_ta... chrome/browser/resources/new_tab.html:162: <div id="arrow_shadow" class="arrow arrow_shadow"></div> On 2011/07/01 00:33:55, Finnur wrote: > On 2011/06/30 23:20:33, Evan Stade wrote: > > why do you have arrow_shadow as both an id and class? > > The class is for .css referencing and the id is to be able to do > GetElementById('arrow_border'). > > GetElementByClassName('arrow_border') doesn't work, probably because the classes > are 'arrow arrow_border' not 'arrow_border', and relying on the class names and > their order not changing seems brittle. you can reference an item by ID in css http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... File chrome/browser/resources/ntp/apps.css (right): http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.css:225: min-width: 200px; how does it grow? If you set width as you have done, it will always be that size. I think you just want min-width and not width? http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... File chrome/browser/resources/ntp/apps.js (right): http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:179: var target = event.target; On 2011/07/01 00:33:55, Finnur wrote: > Not sure what you mean here. event.target and event.currentTarget point to the > same element: the span that is the child of the <a> tag. I need the <a> tag > here. if that is the case, how does this line from handleClick work? var appId = e.currentTarget.getAttribute('app-id'); > > On 2011/06/30 23:20:33, Evan Stade wrote: > > event.currentTarget will do what you want > http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:202: border.style.top = y + "px"; I suggest wrapping all these in a single div so you don't have to set coordinates for all of them http://codereview.chromium.org/7291004/diff/1/chrome/browser/resources/ntp/ap... chrome/browser/resources/ntp/apps.js:208: bubble.style.opacity = 1; On 2011/07/01 00:33:55, Finnur wrote: > I always find it a little silly to create a class around one css property and > start flipping classes around instead of just changing the property. Seems like > the extra level of indirection doesn't result in simpler code. it definitely does if you have the right CSS selectors set up. You'd only have to add the class to one of these things. However you can also shorten this code considerably by following the wrapping div suggestion above. > But, if you feel > strongly about this I can take a look in the morning (it is past midnight here). > > On 2011/06/30 23:20:33, Evan Stade wrote: > > better to do this with a css class than manually editing css in js > http://codereview.chromium.org/7291004/diff/1004/chrome/browser/resources/new... File chrome/browser/resources/new_tab.html (right): http://codereview.chromium.org/7291004/diff/1004/chrome/browser/resources/new... chrome/browser/resources/new_tab.html:159: <a id="app-notification-link"></a></div> nit: place closing div on new line
lgtm if you clean this up after getting back from vacation |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
