1223733 - Migrate the PinPageSystemDialog to the dialog web component

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::System UI, defect)

Description

[Alberto Pastor [:albertopq]]

We want to incrementally move all our system dialogs to webcomponents. This is the first step. As soon as we migrate all of them, the common code should be moved to SystemDialog.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master

Comment 2

[Alberto Pastor [:albertopq]]

Comment on attachment 8690910 [details] [review]
[gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master

Eric, could you please ui-review this? Thanks!

Comment 3

[Eric Pang [:epang] UX]

Comment on attachment 8690910 [details] [review]
[gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master

Looks good, thanks Alberto! R+

Comment 4

[Alberto Pastor [:albertopq]]

Comment on attachment 8690910 [details] [review]
[gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master

Despite a lot of files are changed, most of them are just the result of a bower install inside the shared folder. Michael, would you mind take a look? Thanks!

Comment 5

[Alberto Pastor [:albertopq]]

Comment on attachment 8690910 [details] [review]
[gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master

Wilson, would you mind to review the music/camera upgrade of gaia-icons? Thanks!

Comment 6

[ffos-browser]

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

Comment 7

[Wilson Page [:wilsonpage]]

Comment on attachment 8690910 [details] [review]
[gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master

Looks good, all apps seems to be on gaia-icons v1.0.2 :)

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8690910 [details] [review]
[gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master

I'm very happy to see this happening! \o/

I've left a couple of comments on github. Most notably the one about waiting for the gaia dialog and button components to load before rendering the pin dialog. Also, why are we using gaia-input-text? I see that we've added the component but I can't see where it's being used. Please re-flag me when these small things are addressed.

Also, I have a question for Wilson here. I thought the plan was to allow individual apps to specify their own version of the components they are using, and in doing so no longer rely on shared/elements. Is this still true? If so, this patch is using shared/elements and so we probably need to change this.

Comment 9

[Wilson Page [:wilsonpage]]

(In reply to Michael Henretty [:mhenretty] from comment #8)
> Comment on attachment 8690910 [details] [review]
> [gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master
> 
> I'm very happy to see this happening! \o/
> 
> I've left a couple of comments on github. Most notably the one about waiting
> for the gaia dialog and button components to load before rendering the pin
> dialog. Also, why are we using gaia-input-text? I see that we've added the
> component but I can't see where it's being used. Please re-flag me when
> these small things are addressed.
> 
> Also, I have a question for Wilson here. I thought the plan was to allow
> individual apps to specify their own version of the components they are
> using, and in doing so no longer rely on shared/elements. Is this still
> true? If so, this patch is using shared/elements and so we probably need to
> change this.

I was going to wait until we start migrating components over to NPM before doing this. But we could use Bower inside System app before we've finished this migration. WDYT?

Comment 10

[Alberto Pastor [:albertopq]]

Comment on attachment 8690910 [details] [review]
[gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master

Good catch with the components load! I replied to the other comment in GH.

Regarding the gaia-text-input, is a requirement for gaia-dialog. Despite is not used so far, it will in other dialog types. I can remove it and add it when a dialog with inputs gets migrated. Up to you. Thanks!

Comment 11

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Wilson Page [:wilsonpage] from comment #9)
> I was going to wait until we start migrating components over to NPM before
> doing this. But we could use Bower inside System app before we've finished
> this migration. WDYT?

If it's work we are serious about doing, I say there's not harm in starting now.

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8690910 [details] [review]
[gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master

(In reply to Alberto Pastor [:albertopq] from comment #10)
> Regarding the gaia-text-input, is a requirement for gaia-dialog. Despite is
> not used so far, it will in other dialog types. I can remove it and add it
> when a dialog with inputs gets migrated. Up to you. Thanks!

If it's a dependency then you are right to include it.

I'm seeing a bug. If I hold the phone in portrait mode, open and then close the pin dialog, and then switch to landscape and re-open the pin dialog, the dialog only takes up half the screen. See screenshot I'm about to attach.

Another slightly weird issue is that in RTL mode, Pin Site still appears on the right, and I think to be consistent it should appear on the left. This is kinda nitpicky though.

Comment 13

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: [screenshot] bug

Comment 14

[Alberto Pastor [:albertopq]]

Comment on attachment 8690910 [details] [review]
[gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master

That should be fixed now.

Thanks!

Comment 15

[Michael Henretty [:mikehenrty][:mhenretty]]

Comment on attachment 8690910 [details] [review]
[gaia] albertopq:1223733-migrate-pin-dialog > mozilla-b2g:master

Sadly I don't have time to look at the build again, but the code is fine and if you fixed the bug I mentioned than you have my blessing to land.

Comment 16

[Alberto Pastor [:albertopq]]

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

Comment 17

[ffos-browser]

Ben Francis changed story state to accepted in Pivotal Tracker