1169002 - [User Story] View Pinned Pages

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Ben Francis [:benfrancis]]

As a user I want to see my collection of pinned pages (represented as cards) so I can view a page I saved for later.

Most recently pinned pages should appear at the top.

Comment 1

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/95569344

Comment 2

[Ben Francis [:benfrancis]]

You have a WIP patch for this, right? Should we assign this to you?

Comment 3

[Chris Lord [:cwiiis]]

I'm going to take this for now.

Comment 4

[Ben Francis [:benfrancis]]

Attachment: View Pinned Pages - Specification

Comment 5

[Guillaume Marty [:gmarty]]

Currently stealing this bug from cwiiis. The WIP is in this branch:
https://github.com/gmarty/gaia/tree/Bug-1169002-User-Story-View-Pinned-Pages

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1169002-homescreen-pinned-pages > mozilla-b2g:master

Comment 7

[Chris Lord [:cwiiis]]

Guillaume and I have both been working on this.

Basically ready, just need to add the page indicator and possibly some tests (depends somewhat on existing pin-the-web tests, and given design is in flux, may not be possible right now - basic features are unit tested at least).

Comment 8

[Chris Lord [:cwiiis]]

Comment on attachment 8652828 [details] [review]
[gaia] Cwiiis:bug1169002-homescreen-pinned-pages > mozilla-b2g:master

Hi Alberto - While Guillaume is away, and given we've both worked on this, I think you're the most suitable reviewer. This also touches a few things in shared and system that I think you'll be familiar with.

Everything added is unit-tested, I think - I haven't added marionette tests for the pin card stuff as it relies on system libraries that haven't been written or finalised yet. I'll be sure to file the follow-up for this.

This also exposes a couple of apz bugs that I'll be filing and following up on.

Comment 9

[Alberto Pastor [:albertopq]]

Comment on attachment 8652828 [details] [review]
[gaia] Cwiiis:bug1169002-homescreen-pinned-pages > mozilla-b2g:master

Awesome work! Left some comments in GH. Please, flag me again after taking a look to them.

At the other hand, I noticed that it goes quite easily from the apps panel to the pinned pages panel while scrolling up quickly the apps panel. Is there any way we can avoid vertically scrolling while scrolling horizontally (and viceversa)?

Thanks!

Comment 10

[Chris Lord [:cwiiis]]

(In reply to Alberto Pastor [:albertopq] from comment #9)
> Comment on attachment 8652828 [details] [review]
> [gaia] Cwiiis:bug1169002-homescreen-pinned-pages > mozilla-b2g:master
> 
> Awesome work! Left some comments in GH. Please, flag me again after taking a
> look to them.
> 
> At the other hand, I noticed that it goes quite easily from the apps panel
> to the pinned pages panel while scrolling up quickly the apps panel. Is
> there any way we can avoid vertically scrolling while scrolling horizontally
> (and viceversa)?
> 
> Thanks!

Cheers, will get to the comments! wrt to how easy it is to switch panels, that's an apz bug I think - it seems the frame configuration breaks axis locking. Also breaks vertical overscroll. Will file both with test cases very soon.

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 8652828 [details] [review]
[gaia] Cwiiis:bug1169002-homescreen-pinned-pages > mozilla-b2g:master

Added more unit tests and addressed comments hopefully.

Comment 13

[Alberto Pastor [:albertopq]]

Comment on attachment 8652828 [details] [review]
[gaia] Cwiiis:bug1169002-homescreen-pinned-pages > mozilla-b2g:master

Thanks!

Comment 14

[Alberto Pastor [:albertopq]]

master: https://github.com/mozilla-b2g/gaia/commit/d2605524ba4e5b509425d5de193f05ce3cbbf1cb

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Instead of using mozL10n.get (deprecated), you should use:

this.indicator.setAttribute('data-l10n-id', this.appsVisible ? 'apps-panel' : 'pages-panel'));

and in l10n file:

apps-panel.ariaLabel  = Applications
pages-panel.ariaLabel = Pinned Pages

This way you don't need mozL10n.once or mozL10n.get.

Please, fix it before localizers start translating the strings.

Comment 16

[Chris Lord [:cwiiis]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #15)
> Instead of using mozL10n.get (deprecated), you should use:
> 
> this.indicator.setAttribute('data-l10n-id', this.appsVisible ? 'apps-panel'
> : 'pages-panel'));
> 
> and in l10n file:
> 
> apps-panel.ariaLabel  = Applications
> pages-panel.ariaLabel = Pinned Pages
> 
> This way you don't need mozL10n.once or mozL10n.get.
> 
> Please, fix it before localizers start translating the strings.

Sure thing. It would be good if this was documented here: https://developer.mozilla.org/en-US/Apps/Build/Localization/Localizing_Firefox_OS_Apps#The_get_Method_%28deprecated%29

Leaving n?me until I get to fixing the bug.

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] Cwiiis:bug1169002-followup-new-homescreen-pinned-pages-localization > mozilla-b2g:master

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8657796 [details] [review]
[gaia] Cwiiis:bug1169002-followup-new-homescreen-pinned-pages-localization > mozilla-b2g:master

Thanks! That looks great :)

Comment 19

[Chris Lord [:cwiiis]]

Thanks for the review, merged: https://github.com/mozilla-b2g/gaia/commit/58adcef3f0628951a156631ccab2d8b35c9d61da

Comment 20

[Adrian Aichner [:anaran]]

(In reply to Chris Lord [:cwiiis] from comment #16)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #15)
> > Instead of using mozL10n.get (deprecated), you should use:
> > 
> > this.indicator.setAttribute('data-l10n-id', this.appsVisible ? 'apps-panel'
> > : 'pages-panel'));
> > 
> > and in l10n file:
> > 
> > apps-panel.ariaLabel  = Applications
> > pages-panel.ariaLabel = Pinned Pages
> > 
> > This way you don't need mozL10n.once or mozL10n.get.
> > 
> > Please, fix it before localizers start translating the strings.
> 
> Sure thing. It would be good if this was documented here:
> https://developer.mozilla.org/en-US/Apps/Build/Localization/
> Localizing_Firefox_OS_Apps#The_get_Method_%28deprecated%29
> 
> Leaving n?me until I get to fixing the bug.

It is documented in the same document at
https://developer.mozilla.org/en-US/Apps/Build/Localization/Localizing_Firefox_OS_Apps#Localizing_HTML_from_JavaScript.3A_Best_practice

There is also
https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices

Would be good if this duplicated documentation could be unified.

For now I put in links to those places under
https://developer.mozilla.org/en-US/Apps/Build/Localization/Localizing_Firefox_OS_Apps#The_get_Method_%28deprecated%29

Comment 21

[Adrian Aichner [:anaran]]

Bug 1202658 covers the duplication issue.

Comment 22

[Chris Lord [:cwiiis]]

Ah yes, I see that second document has the information about ariaLabel in the properties file (which is the bit I was missing).