Closed
Bug 809308
Opened 12 years ago
Closed 12 years ago
Add an option to the download prompt to download only the system update
Categories
(Firefox OS Graveyard :: Gaia::System, defect, P1)
Tracking
(blocking-basecamp:+)
People
(Reporter: etienne, Assigned: etienne)
References
Details
(Keywords: late-l10n)
Attachments
(2 files, 5 obsolete files)
We want to let the user decide to download only the system update on the download prompt. UX needs to be defined.
Assignee | ||
Updated•12 years ago
|
blocking-basecamp: --- → ?
This is a requirement from product. Though if this turns out to be too hard, or takes too long to land, then I would say that we should reconsider blocking.
blocking-basecamp: ? → +
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → etienne
Comment 2•12 years ago
|
||
This is critical for users to be able to stay up to date on the latest version. We do not want to block OS updates if a user doesn't want to update a 3rd party application. Josh, can you provide the link to the UX spec for this? Thanks.
Priority: -- → P1
Updated•12 years ago
|
Component: Gaia → Gaia::System
Assignee | ||
Updated•12 years ago
|
Comment 3•12 years ago
|
||
I don't understand why this doesn't fit in the system component. I thought it did?
Component: Gaia → Gaia::System
Priority: -- → P1
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #3) > I don't understand why this doesn't fit in the system component. I thought > it did? It does. Not sure what happened.
Comment 5•12 years ago
|
||
Milestoning for C2 (deadline of 12/10), as this meets the criteria of "remaining P1 bugs not already milestoned for C1".
Target Milestone: --- → B2G C2 (20nov-10dec)
Assignee | ||
Comment 6•12 years ago
|
||
final spec for this screen
Assignee | ||
Comment 7•12 years ago
|
||
Had to fix a small bug along the way to enable me to test this correctly (as in manual tests). Hope you won't mind.
Attachment #684511 -
Flags: review?(felash)
Comment 8•12 years ago
|
||
Comment on attachment 684511 [details] [diff] [review] Patch proposal Review of attachment 684511 [details] [diff] [review]: ----------------------------------------------------------------- I suspect something's missing: if nothing is selected, "Download" should be disabled. Good job ! ::: apps/system/js/update_manager.js @@ +172,2 @@ > var listItem = document.createElement('li'); > listItem.textContent = updatable.name; I'd really put the name inside the label : - immediate benefit: the user can tap the name of the application to check the box. Big utilisability win ! - free future accessibility benefit (when we'll have a screen reader) However we can open another bug if that's too much CSS work. (I'd rather not because I'm sure it would be done like in 1 year) @@ +172,4 @@ > var listItem = document.createElement('li'); > listItem.textContent = updatable.name; > > + // The user can choose not to update an app OMG a comment ! (that's ok, you'll be fine) @@ +172,5 @@ > var listItem = document.createElement('li'); > listItem.textContent = updatable.name; > > + // The user can choose not to update an app > + if (updatable.app) { why not use instanceof AppUpdatable ? seems better than using internal properties from AppUpdatable. Better: why not put this inside AppUpdatable and SystemUpdatable, and you'd just have to call "renderDownloadBox" (or something like that) on the Updatable implementation? @@ +180,5 @@ > + checkbox.type = 'checkbox'; > + checkbox.dataset.position = index; > + checkbox.checked = true; > + > + var span = document.createElement('span'); what is this |span| for ? @@ +192,5 @@ > + label.textContent = _('required'); > + label.classList.add('required'); > + > + listItem.appendChild(label); > + } you can put the "createElement" and "appendChild" calls out of the |if| block. ::: apps/system/locales/system.en-US.properties @@ +95,4 @@ > updatesAvailableMessage[many]={{ n }} updates available. <span>Tap to download.</span> > updatesAvailableMessage[other]={{ n }} updates available. <span>Tap to download.</span> > updates={[ plural(n) ]} > +updates[one]={{ n }} Update available I'd put "One update available" @@ +98,5 @@ > +updates[one]={{ n }} Update available > +updates[two]={{ n }} Updates available > +updates[few]={{ n }} Updates available > +updates[many]={{ n }} Updates available > +updates[other]={{ n }} Updates available I'd say we should remove the capital letter in Updates. @@ +103,2 @@ > systemUpdate=System update > +required=Required you can remove the capital since you're capitalizing everything in CSS anyway ::: apps/system/style/update_manager/update_manager.css @@ +116,5 @@ > pointer-events: auto; > } > > +#updates-download-dialog h1 { > + font-size: 2.2rem; why do we need specifically this for this specific dialog ? @@ +167,5 @@ > + float: right; > + > + line-height: 5rem; > + font-size: 1.3rem; > + text-transform: uppercase; Maybe move the transform to the next rule. ::: apps/system/test/unit/update_manager_test.js @@ +637,5 @@ > + appUpdatable.name = 'Angry birds'; > + appUpdatable.size = '423459'; > + > + UpdateManager.updatesQueue = [appUpdatable, > + systemUpdatable]; why can't you use addToUpdatesQueue ? @@ +639,5 @@ > + > + UpdateManager.updatesQueue = [appUpdatable, > + systemUpdatable]; > + > + UpdateManager.containerClicked(); nitpicking here, but couldn't you try to use "click()" on the container DOM node ? @@ +740,4 @@ > var item = UpdateManager.downloadDialogList.children[1]; > + var expectedMarkup = ['Angry birds', > + '<label>', > + '<input data-position="1" type="checkbox">', I'm wondering if this is right.. because depending on what gecko does, attributes could be swapped for example. unless this is well specified and well implemented in gecko. And I don't think using DOM methods would be less readable actually.
Attachment #684511 -
Flags: review?(felash) → review-
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Julien Wajsberg [:julienw] [:everlong] from comment #8) > I'd really put the name inside the label : > > - immediate benefit: the user can tap the name of the application to check > the box. Big utilisability win ! > - free future accessibility benefit (when we'll have a screen reader) > > However we can open another bug if that's too much CSS work. (I'd rather not > because I'm sure it would be done like in 1 year) I gave it a fair shot but I'm afraid it can not be done without modifying the swiches.css building block, which is way outside the scope here. > > @@ +180,5 @@ > > + checkbox.type = 'checkbox'; > > + checkbox.dataset.position = index; > > + checkbox.checked = true; > > + > > + var span = document.createElement('span'); > > what is this |span| for ? For the building block :) > > ::: apps/system/style/update_manager/update_manager.css > @@ +116,5 @@ > > pointer-events: auto; > > } > > > > +#updates-download-dialog h1 { > > + font-size: 2.2rem; > > why do we need specifically this for this specific dialog ? It doesn't have the same design, the design decision for the bigger font probably comes from the fact that the content is scrollable here.
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #684511 -
Attachment is obsolete: true
Attachment #684700 -
Flags: review?(felash)
Assignee | ||
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
Comment on attachment 684700 [details] [diff] [review] Patch v2 Review of attachment 684700 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/system/js/update_manager.js @@ +173,5 @@ > listItem.textContent = updatable.name; > > + // The user can choose not to update an app > + var checkContainer = document.createElement('label'); > + if (updatable instanceof AppUpdatable) { I still don't like that and you know it, but I won't hold back this patch for this. @@ +177,5 @@ > + if (updatable instanceof AppUpdatable) { > + var checkbox = document.createElement('input'); > + checkbox.type = 'checkbox'; > + checkbox.dataset.position = index; > + checkbox.checked = true; You didn't see my global comment so I put it here instead: I suspect something's missing here: if nothing is selected, "Download" should be disabled. So we need to bind a onchange event on the form that would check if every checkbox is checked. ::: apps/system/test/unit/update_manager_test.js @@ +732,4 @@ > var item = UpdateManager.downloadDialogList.children[0]; > + var expectedMarkup = ['systemUpdate', > + '<label class="required">required</label>', > + '<div>5.05 MB</div>']; you don't use this markup, do you ?
Attachment #684700 -
Flags: review?(felash) → review-
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #684700 -
Attachment is obsolete: true
Attachment #684701 -
Attachment is obsolete: true
Attachment #685105 -
Flags: review?(felash)
Comment 14•12 years ago
|
||
Comment on attachment 685105 [details] [diff] [review] Patch proposal v3 Review of attachment 685105 [details] [diff] [review]: ----------------------------------------------------------------- we're getting there ::: apps/system/js/update_manager.js @@ +17,4 @@ > _downloadedBytes: 0, > _errorTimeout: null, > _wifiLock: null, > + _systemUpdateDisplayed: false, I would choose "_hasSystemUpdate" instead. @@ +65,5 @@ > > this.container.onclick = this.containerClicked.bind(this); > this.laterButton.onclick = this.cancelPrompt.bind(this); > + this.downloadButton.onclick = this.startDownloads.bind(this); > + this.downloadDialogList.onclick = this.updateDownloadButton.bind(this); did you try with "onchange" instead of "onclick" ? @@ +176,5 @@ > listItem.textContent = updatable.name; > > + // The user can choose not to update an app > + var checkContainer = document.createElement('label'); > + if (updatable instanceof AppUpdatable) { I'd be more comfortable if this test was reversed : the positive part of the |if| is for SystemUpdatable because this is the exception. @@ +219,5 @@ > + var dialog = this.downloadDialogList; > + var checkboxes = dialog.querySelectorAll('input[type="checkbox"]'); > + for (var i = 0; i < checkboxes.length; i++) { > + if (checkboxes[i].checked) { > + disabled = false; you could |break| here. ::: apps/system/locales/system.en-US.properties @@ +94,4 @@ > updatesAvailableMessage[few]={{ u }} updates available. <span>Tap to download.</span> > updatesAvailableMessage[many]={{ n }} updates available. <span>Tap to download.</span> > updatesAvailableMessage[other]={{ n }} updates available. <span>Tap to download.</span> > +numberOfUpdates={[ plural(n) ]} be careful as this applies with an offset, you should rebase ::: apps/system/style/update_manager/update_manager.css @@ +156,2 @@ > display: block; > + width: calc(100% - 12rem); isn't it possible to use a simple padding here ? div always use 100% anyway so it would be simpler. (if this works, I'm not so good in CSS ;) ) ::: apps/system/test/unit/update_manager_test.js @@ -632,3 @@ > UpdateManager.init(); > > - var evt = document.createEvent('MouseEvents'); I'd like to see if using "git format-patch --patience" would take this line in the diff (because it's still there after the patch). Just out of curiosity :) @@ +699,5 @@ > + UpdateManager.removeFromUpdatesQueue(systemUpdatable); > + UpdateManager.container.click(); > + > + evt = document.createEvent('MouseEvents'); > + evt.initEvent('click', true, true); do you use this ?
Attachment #685105 -
Flags: review?(felash) → review-
Assignee | ||
Comment 15•12 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #14) > ::: apps/system/style/update_manager/update_manager.css > @@ +156,2 @@ > > display: block; > > + width: calc(100% - 12rem); > > isn't it possible to use a simple padding here ? > > div always use 100% anyway so it would be simpler. (if this works, I'm not > so good in CSS ;) ) Nope it's pushing the "required" label away. > > @@ +699,5 @@ > > + UpdateManager.removeFromUpdatesQueue(systemUpdatable); > > + UpdateManager.container.click(); > > + > > + evt = document.createEvent('MouseEvents'); > > + evt.initEvent('click', true, true); > > do you use this ? Yep I want the dialog to be re-rendered. Patch incoming...
Assignee | ||
Comment 16•12 years ago
|
||
Crossing fingers...
Attachment #685105 -
Attachment is obsolete: true
Attachment #685299 -
Flags: review?(felash)
Comment 17•12 years ago
|
||
Comment on attachment 685299 [details] [diff] [review] Patch v4 Review of attachment 685299 [details] [diff] [review]: ----------------------------------------------------------------- r=me thanks for this nice UI :) ::: apps/system/test/unit/update_manager_test.js @@ +699,5 @@ > + UpdateManager.removeFromUpdatesQueue(systemUpdatable); > + UpdateManager.container.click(); > + > + evt = document.createEvent('MouseEvents'); > + evt.initEvent('click', true, true); you're not using |evt| in this suite.
Attachment #685299 -
Flags: review?(felash) → review+
Assignee | ||
Comment 18•12 years ago
|
||
Here is the final patch.
Attachment #685299 -
Attachment is obsolete: true
Attachment #685543 -
Flags: review?(stas)
Attachment #685543 -
Flags: review?(felash)
Comment 19•12 years ago
|
||
Comment on attachment 685543 [details] [diff] [review] Final patch r=me
Attachment #685543 -
Flags: review?(felash) → review+
Comment 20•12 years ago
|
||
Comment on attachment 685543 [details] [diff] [review] Final patch r=me on the localization part, thanks!
Attachment #685543 -
Flags: review?(stas) → review+
Assignee | ||
Comment 21•12 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/6adc6c7b76186164359d8fa40b4d682cb4a6bc0c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•