Create gaia-sim-picker component

RESOLVED FIXED in 2.1 S5 (26sep)

Status

Firefox OS
Gaia
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: kgrandon, Assigned: drs)

Tracking

unspecified
2.1 S5 (26sep)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [planned-sprint c=6][in-sprint=v2.1-S2])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

4 years ago
Essentially the following files should be moved and componentized:

shared/js/sim_picker.js -> elements/sim_picker/script.js
shared/js/elements/sim_picker.html -> also to script.js, just until we start using HTML imports or have a better templating solution.
shared/locales/sim_picker/sim_picker.en-US.properties -> shared/elements/sim_picker/locales/sim_picker.en-US.properties
(Reporter)

Comment 1

4 years ago
Doug - I noticed that you did the original sim-picker and since this would be a really good candidate for a web component. I wanted to check with you to see if it was something you'd be interested in working with us on for 2.1. If you have time to port this that would be awesome. If you don't have time, but have time for some reviews, that would also be great. 

I suppose it is most similar to the gaia-menu component[1]. We could probably leverage gaia-menu directly, or extend it in some way. Let me know what you think.

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_menu/script.js
Flags: needinfo?(drs+bugzilla)
(Assignee)

Comment 2

4 years ago
Sure, I can help with and review this. I don't know much about making shadow DOM elements so I'd probably need help there.

Have you thought at all about the design? Right now, it's pretty tightly integrated into the JS of whatever is using it. I think we'd have to move a lot of these things to attrs and DOM events. I'm not sure how we'd replace the getOrPick function, though. Do you have any example code which is similar (gaia-menu doesn't seem to be)?
Flags: needinfo?(drs+bugzilla)
(Reporter)

Comment 3

4 years ago
Hmm, the gaia-menu is probably the closest thing we currently have I imagine. I suppose a lot of this would be the first time we're trying to do this from a component, and I'm only vaguely familiar with the functionality of the sim picker.

Looking briefly at the getOrPick, it would be no problem to surface this through a custom API on the component if we wanted, or we could do something more complex and have a type of data layer though I'm not sure if the extra complexity is necessary. For now I think that moving it into an API on the component would be ok? Also maybe the component could simply fire a custom event when an option is picked, or if there is only one option.
(Assignee)

Comment 4

4 years ago
Yeah, that seems reasonable. I'm not sure how to expose a custom API from a component like you're talking about though, so an example of that would be helpful. If this is indeed the first time we're doing something like this, then even a rough outline or thought process would be helpful too.

I'll assign this to myself for now and get to it when I can (probably next sprint).
Assignee: nobody → drs+bugzilla
(Reporter)

Comment 5

4 years ago
(In reply to Doug Sherk (:drs) from comment #4)
> Yeah, that seems reasonable. I'm not sure how to expose a custom API from a
> component like you're talking about though, so an example of that would be
> helpful. 

We're still figuring out patterns, so nothing set in stone, but here's a few examples of custom APIs on components: 

https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/script.js#L52
https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_menu/script.js#L37


The confirm component also fires a custom event on the element, which is what I assume we would want to do for the sim-picker. Maybe some`sim-picker-select` event would be fired when the user selects a sim? https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_confirm/script.js#L63

Thanks for taking this up, the more people that experiment with web components the better :)
(Assignee)

Comment 6

4 years ago
Okay, thanks. I think I have a reasonable idea of where to go with this now.
(Assignee)

Updated

4 years ago
Whiteboard: [planned-sprint]
Target Milestone: --- → 2.1 S1 (1aug)
(Assignee)

Updated

3 years ago
Whiteboard: [planned-sprint] → [planned-sprint c=3]
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 7

3 years ago
We've been keeping notes on this and my feedback on WC so far here:
https://etherpad.mozilla.org/web-components
(Assignee)

Comment 8

3 years ago
WIP PR: https://github.com/mozilla-b2g/gaia/pull/22129

I'm stuck at this point with the shadow DOM not appearing when I call ComponentUtils.style.call(). For some reason, without this call, it works. I can also move the shadow DOM to the same level as the shadow host and it appears.

Kevin, you said you'd take a look into this, so I'm needinfoing you. Thanks for your help so far.
Flags: needinfo?(kgrandon)
(Reporter)

Comment 9

3 years ago
Hey Doug - I'll take a look soon. I'm currently traveling with only 1 sim - is there a way to view this on a device or desktop with only 1 sim? I think if we can implement a small html test page to load in the browser it might make it easy to debug - but it looks like we would need to add some shims/mocks for the mobile APIs.

I also left a note about localization files, looking good so far. Thanks!
Flags: needinfo?(kgrandon)
(Assignee)

Comment 10

3 years ago
Ok, I'll make a reduced test case and give you a patch to get it working with 1 SIM.
Flags: needinfo?(drs+bugzilla)
(Assignee)

Comment 11

3 years ago
The PR now has an example which also has this problem. I included a few lines you can use to verify the problem:
https://github.com/mozilla-b2g/gaia/pull/22129/files#diff-b11c46e499ed8813cee5a027fa257ae7R30
Flags: needinfo?(drs+bugzilla) → needinfo?(kgrandon)
(Reporter)

Comment 12

3 years ago
(In reply to Doug Sherk (:drs) from comment #11)
> The PR now has an example which also has this problem. I included a few
> lines you can use to verify the problem:
> https://github.com/mozilla-b2g/gaia/pull/22129/files#diff-
> b11c46e499ed8813cee5a027fa257ae7R30

I took a look at the example and noticed that the <content> seemed a bit weird as we aren't using it as an insertion point. Removing this seems to get content to show up for me. Left a comment on github as well.
Flags: needinfo?(kgrandon)
(Assignee)

Updated

3 years ago
Blocks: 1045820
(Assignee)

Comment 13

3 years ago
Created attachment 8464263 [details] [diff] [review]
Create gaia-sim-picker component

This seems to work pretty well. I left one comment for a styling problem that I need to come back to. I also need to write/port tests.
Attachment #8464263 - Flags: feedback?(kgrandon)
(Assignee)

Comment 14

3 years ago
I also updated the PR: https://github.com/mozilla-b2g/gaia/pull/22129
(Reporter)

Comment 15

3 years ago
Comment on attachment 8464263 [details] [diff] [review]
Create gaia-sim-picker component

Review of attachment 8464263 [details] [diff] [review]:
-----------------------------------------------------------------

::: shared/elements/gaia_sim_picker/examples/example.js
@@ +9,5 @@
> +    for (var i = 0; i < showSimPickerButtons.length; i++) {
> +      var showSimPickerButton = showSimPickerButtons[i];
> +      showSimPickerButton.addEventListener('click', function(e) {
> +        simPicker.getOrPick(e.target.dataset.defaultCardIndex, '12345',
> +        function(cardIndex) {

I think you should do whatever will make the transition easier, but what do you think about surfacing this as an event on the sim picker element? E.g, 

var simPicker = document.getElementById('sim-picker');
simPicker.addEventListener('pick', onPick);

::: shared/elements/gaia_sim_picker/script.js
@@ +49,5 @@
> +    }
> +
> +    var dialViaElt = this.shadowRoot.querySelector('#sim-picker-dial-via');
> +    if (phoneNumber) {
> +      navigator.mozL10n.setAttributes(dialViaElt,

I think the only difference is that this also sets the l10n-id and l10n-args properties? Personally I'd like to remove the hard mozL10n dependency if possible, but we can always do this later if that's easier. If we did want to remove it, I guess we would just call setAttribute() for both data-l10n-id and data-l10n-args.
Attachment #8464263 - Flags: feedback?(kgrandon) → feedback+
(Reporter)

Comment 16

3 years ago
Oops - forgot an "Overall" comment there :) Very happy with this, thanks for experimenting. I left a few comments, but I couldn't find anything serious that would prevent this from landing as-is. Thanks for taking care of this!
(Assignee)

Comment 17

3 years ago
Created attachment 8464935 [details] [diff] [review]
Create gaia-sim-picker component.

PR: https://github.com/mozilla-b2g/gaia/pull/22129

bug 1045820 is getting rapidly out of control and has turned into a port of every app, so it'll come a bit later.
Attachment #8464263 - Attachment is obsolete: true
Attachment #8464935 - Flags: review?(kgrandon)
Attachment #8464935 - Flags: review?(anthony)
(Assignee)

Comment 18

3 years ago
(In reply to Kevin Grandon :kgrandon from comment #15)
> I think the only difference is that this also sets the l10n-id and l10n-args
> properties? Personally I'd like to remove the hard mozL10n dependency if
> possible, but we can always do this later if that's easier. If we did want
> to remove it, I guess we would just call setAttribute() for both
> data-l10n-id and data-l10n-args.

I forgot to mention that I disagree with this. We still have other dependencies on l10n, such as the ready() and once() functions. I also could easily imagine other WC needing these stubs.
(Reporter)

Comment 19

3 years ago
Awesome, no harm in landing this now and iterating when needed while you start porting apps. Thanks for putting this together, I'll start reviewing soon.

(In reply to Doug Sherk (:drs) from comment #18)
> I forgot to mention that I disagree with this. We still have other
> dependencies on l10n, such as the ready() and once() functions. I also could
> easily imagine other WC needing these stubs.

Gotcha. The l10n + components land is changing frequently, so I'm not sure what will be best down the road. Totally fine with this for now.
(Reporter)

Comment 20

3 years ago
Comment on attachment 8464935 [details] [diff] [review]
Create gaia-sim-picker component.

Review of attachment 8464935 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I'm pretty happy with this :) I think I would like to let Anothony take a look, and maybe some of my comments addressed before leaving an R+. Thanks for knocking out, I think we're close here.

::: apps/communications/dialer/test/unit/gaia_sim_picker_test.js
@@ +1,1 @@
> +/* globals MocksHelper, MockNavigatorMozIccManager, MockL10n,

I'm not too familiar with the sim picker, is the only logic to test difference when we have and don't have TelephonyHelper?

::: shared/elements/gaia_sim_picker/locales/gaia_sim_picker.en-US.properties
@@ +1,2 @@
> +# SIM list
> +sim-picker-dial-via-with-number=Dial {{ phoneNumber }} via

Would you be ok with going with new keys here? We currently prefix all of our component strings with the component name itself, potentially to avoid collisions with apps, and I think it would be nice to keep it consistent. The only change here would be to prefix these with 'gaia-'. E.g., gaia-sim-picker-dial-via-with-number

https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/locales/gaia_grid.en-US.properties

If you are ok with this, I'll add it the web component README.md which I'm putting together now.

::: shared/elements/gaia_sim_picker/script.js
@@ +30,5 @@
> +  };
> +
> +  proto._domBuilt = false;
> +
> +  proto.localize = function() {

Seems wrong to have an empty function here. Do we need to do any localization here? If not, let's remove this.

::: shared/elements/gaia_sim_picker/style.css
@@ +1,3 @@
> +.sim-default {
> +  display: none;
> +  margin-left: 2px;

Should we use rem here?
Attachment #8464935 - Flags: review?(kgrandon)
Comment on attachment 8464935 [details] [diff] [review]
Create gaia-sim-picker component.

Removing review to clear my queue. Ask again when Kevin's comments are addressed.
Attachment #8464935 - Flags: review?(anthony)
(Assignee)

Comment 22

3 years ago
Created attachment 8465963 [details] [diff] [review]
Create gaia-sim-picker component.

Thanks for the review.

(In reply to Kevin Grandon :kgrandon from comment #20)
> I'm not too familiar with the sim picker, is the only logic to test
> difference when we have and don't have TelephonyHelper?

Yes. TelephonyHelper is specific to the dialer, though. Maybe we could move mock_telephony_helper.js to shared/test/unit/mocks/dialer/

I had to make some additional changes here to support bug 1045820. Unfortunately I squashed them, but I think the only major thing is the addition of ElementMocksHelper.

Updated PR.
Attachment #8464935 - Attachment is obsolete: true
Attachment #8465963 - Flags: review?(kgrandon)
Attachment #8465963 - Flags: review?(anthony)
(Assignee)

Comment 23

3 years ago
Switching to using an event instead of a callback passed into getOrPick has caused problems in the apps using this. For example, this is really ugly:
https://github.com/DouglasSherk/gaia/commit/3c5da16a5d2cb12e6a1b9f35eb5608603a04f019#diff-98e96f21adf938206005ba91e4984e69R628

It has also made our testing much more difficult. Maybe we could do both instead.
(Reporter)

Comment 24

3 years ago
(In reply to Doug Sherk (:drs) from comment #23)
> Switching to using an event instead of a callback passed into getOrPick has
> caused problems in the apps using this. For example, this is really ugly:
> https://github.com/DouglasSherk/gaia/commit/
> 3c5da16a5d2cb12e6a1b9f35eb5608603a04f019#diff-
> 98e96f21adf938206005ba91e4984e69R628
> 
> It has also made our testing much more difficult. Maybe we could do both
> instead.

Ok, thanks for experimenting. If you feel a callback is the way to go, that works for me. I guess I was just thinking that you'd *always* have the event listener active and listening, but maybe that isn't too nice either.
(Reporter)

Comment 25

3 years ago
Comment on attachment 8465963 [details] [diff] [review]
Create gaia-sim-picker component.

Review of attachment 8465963 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is a great start. We will undoubtedly need some API changes and react to changes in our component strategy - but we will tackle those when we get to them. Nice work!

::: shared/test/unit/mocks/elements/gaia_sim_picker/mock_gaia_sim_picker.js
@@ +1,1 @@
> +/* exported MockGaiaSimPicker */

Is this file currently loaded anywhere? I couldn't find a reference to it, but maybe I missed it. My preference would be to still load the real custom element for tests, but I guess not everyone is a fan of that approach.

In any case, if it's not referenced, should we skip landing it?
Attachment #8465963 - Flags: review?(kgrandon) → review+
(Assignee)

Comment 26

3 years ago
(In reply to Kevin Grandon :kgrandon from comment #24)
> Ok, thanks for experimenting. If you feel a callback is the way to go, that
> works for me. I guess I was just thinking that you'd *always* have the event
> listener active and listening, but maybe that isn't too nice either.

I want to fix this first before we land it. It'll make bug 1045280 a bit easier. We can land this after I've done that.

(In reply to Kevin Grandon :kgrandon from comment #25)
> ::: shared/test/unit/mocks/elements/gaia_sim_picker/mock_gaia_sim_picker.js
> @@ +1,1 @@
> > +/* exported MockGaiaSimPicker */
> 
> Is this file currently loaded anywhere? I couldn't find a reference to it,
> but maybe I missed it. My preference would be to still load the real custom
> element for tests, but I guess not everyone is a fan of that approach.
> 
> In any case, if it's not referenced, should we skip landing it?

Yeah, it's used in bug 1045280. It's loaded by ElementMocksHelper in a couple of tests. It's not used directly.
(Assignee)

Updated

3 years ago
Whiteboard: [planned-sprint c=3] → [planned-sprint c=3][in-sprint=v2.1-S1]
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
(Assignee)

Comment 27

3 years ago
Kevin, I could use your help in a couple of spots. We can chat on IRC too if that's better for you. These are the last things I have to deal with before this will be completely done, including bug 1045280 (other than review).

PR: https://github.com/mozilla-b2g/gaia/pull/22386

1) The shadow DOM looks roughly like this:
<gaia-sim-picker>
  <gaia-menu>
    <buttons></buttons>
  </gaia-menu>
</gaia-sim-picker>

It seems that the reason the menu is not being formatted correctly without my fix [1] is because the nested gaia-menu element is not actually being initialized. The dependencies are definitely being loaded, because I can add a gaia-menu element here [2] and that works fine.

Inspecting the shadow DOM in Chrome confirms this. The <gaia-menu> element has no underlying shadow DOM. Do nested elements not work yet or something?

2) When I show or hide the SIM picker, the first click event after either of these seems to get ignored in content. I immediately thought that this was a focus issue, but after trying to set focus on the element being shown or blurring it when it's hidden, it doesn't seem to be. I removed the preventDefault from the 'contextmenu' event handler on the buttons that show the SIM picker, and that fixed it. But this fix is unacceptable because it triggers clicks inside the SIM picker itself when you lift your finger from the button that shows it. This wasn't a problem in the past. Do you know anything about this?

Thanks.

[1] https://github.com/mozilla-b2g/gaia/pull/22386/files#diff-7a208b07a721f29598b7d72c5d461cd0R11
[2] https://github.com/mozilla-b2g/gaia/pull/22386/files#diff-98bf553eb11786402e0627509cbd9974R28
Flags: needinfo?(kgrandon)
(Reporter)

Comment 28

3 years ago
(In reply to Doug Sherk (:drs) from comment #27)
> 1) The shadow DOM looks roughly like this:...
> It seems that the reason the menu is not being formatted correctly without
> my fix [1] is because the nested gaia-menu element is not actually being
> initialized. The dependencies are definitely being loaded, because I can add
> a gaia-menu element here [2] and that works fine.
> 
> Inspecting the shadow DOM in Chrome confirms this. The <gaia-menu> element
> has no underlying shadow DOM. Do nested elements not work yet or something?

This is strange. I recall the menu working properly before, but opening the example in nightly today seems to have the messed up formatting. Did anything change in your code? If nothing changed it could be a platform bug. If that's the case we should see if we can get a reduced test case and have the platform guys look at it. In either case we should explore some workarounds. One option might be to stop using <gaia-menu> or try surfacing it through a <content select="gaia-menu"> tag instead. I'll take a look and see if there's any workarounds I can easily find.


> 2) When I show or hide the SIM picker, the first click event after either of
> these seems to get ignored in content. I immediately thought that this was a
> focus issue, but after trying to set focus on the element being shown or
> blurring it when it's hidden, it doesn't seem to be. I removed the
> preventDefault from the 'contextmenu' event handler on the buttons that show
> the SIM picker, and that fixed it. But this fix is unacceptable because it
> triggers clicks inside the SIM picker itself when you lift your finger from
> the button that shows it. This wasn't a problem in the past. Do you know
> anything about this?

Have we tried using a stopPropagation() instead of preventDefault()?
(Assignee)

Comment 29

3 years ago
(In reply to Kevin Grandon :kgrandon from comment #28)
> This is strange. I recall the menu working properly before, but opening the
> example in nightly today seems to have the messed up formatting. Did
> anything change in your code? If nothing changed it could be a platform bug.

Oh, I disabled my fix [1] so that you could see the brokenness. But my point here is that the underlying problem is more serious than I originally thought, so my CSS hack isn't a valid fix.

[1] https://github.com/mozilla-b2g/gaia/pull/22386/files#diff-7a208b07a721f29598b7d72c5d461cd0R11

> If that's the case we should see if we can get a reduced test case and have
> the platform guys look at it. In either case we should explore some
> workarounds. One option might be to stop using <gaia-menu> or try surfacing
> it through a <content select="gaia-menu"> tag instead. I'll take a look and
> see if there's any workarounds I can easily find.

I suspect that this isn't a platform bug since it happens in Chrome as well.

> Have we tried using a stopPropagation() instead of preventDefault()?

I just tried that and stopImmediatePropagation() and neither fixed it, unfortunately. This would be a bad fix anyways since it would depend on call ordering.
(Reporter)

Comment 30

3 years ago
Ok, thanks Doug. I think the next steps here are to narrow down a test case to check if this is a platform bug, and what if any workarounds we have available.

I'll try to help out this week and investigate.
Flags: needinfo?(kgrandon)
(Assignee)

Comment 31

3 years ago
I wrote a reduced test case for this. It seems that the problem is our template/cloneNode() logic.

Here it is broken, using template.cloneNode():
http://people.mozilla.org/~dsherk/shadow-test-broken.html

Here it is working, just using shadow.innerHTML = /* ... */:
http://people.mozilla.org/~dsherk/shadow-test-working.html
(Reporter)

Comment 32

3 years ago
Here the example is in jsfiddle in case we want to play with some live-editing of the example: http://jsfiddle.net/kevining/1b3zmnts/
(Reporter)

Comment 33

3 years ago
William - if you have a moment, could you look at this link: http://jsfiddle.net/kevining/1b3zmnts/

We currently have a nested web component that is not getting instantiated for some reason. It does not work in Chrome as well, so I'm wondering if we're doing something crazy in violation of the spec. Could you take a look and see if we're doing something dumb? Thanks!
Flags: needinfo?(wchen)
So it looks like what's happening in the test case is this:

- Elements in a template contents are owned by a special document called the "template contents owner" (we do this to try and ensure that template contents stay inert and don't affect the main document).
- Per spec, the custom elements registry is empty for the template contents owner. [1]
- When you clone the template contents, the cloned nodes are created in the template contents owner document.
- During the creation of the cloned elements, we don't have a custom element registration in the template contents owner document, so the created callback does not get called and the custom prototype is not set on the element. [2]
- The cloned node then gets adoped into the main document.

One of the issues we have here is that we are adopting a custom element from a document that does not have a custom element definition into a document where it does. This scenerio currently isn't worked out yet at the spec level because there are other complications (e.g. what prototype and callbacks do we use when the document we are adopting from and the document we are adopting into both have different custom element defintions?)

[1] http://w3c.github.io/webcomponents/spec/custom/#creating-and-passing-registries
[2] http://w3c.github.io/webcomponents/spec/custom/index.html#dfn-created-callback
Flags: needinfo?(wchen)
(Assignee)

Comment 35

3 years ago
I see. So it seems like there's no way to do this right now using template elements. The only reasonable alternative I can think of is to just set the innerHTML directly, similar to my test case. We can fix this properly when we can load HTML fragments in WC. Does this seem reasonable?
(Reporter)

Comment 36

3 years ago
Thanks for digging into this William, that was really insightful! So it seems like we may be able to get around this by using a direct injection into the shadow root, instead of using a template? This might be ok for now, but seems like it might be limiting should we ever decide to use HTML imports.

Anyway - I'll follow-up with you about how we want to track this spec work, and look at some workarounds with Doug here. I'll remove you from the CC here as it will go back to typical bug-churn I think, but feel free to add yourself :) Thanks!
(Reporter)

Comment 37

3 years ago
(In reply to Doug Sherk (:drs) from comment #35)
> I see. So it seems like there's no way to do this right now using template
> elements. The only reasonable alternative I can think of is to just set the
> innerHTML directly, similar to my test case. We can fix this properly when
> we can load HTML fragments in WC. Does this seem reasonable?

Yes, it seems reasonable to set innerHTML of the shadowRoot directly. This workaround seems to do the trick for me: http://jsfiddle.net/kevining/atwwjpng/
(Assignee)

Comment 38

3 years ago
Created attachment 8472013 [details] [diff] [review]
Create gaia-sim-picker component.

Updated PR.

Ok, here it is with the innerHTML/template workaround. This also fixed the focus issue that I described in comment 27, so this is now completely ready to go. I also have a patch ready for bug 1045280 which I'll post and ask to be reviewed once we've landed this.
Attachment #8465963 - Attachment is obsolete: true
Attachment #8465963 - Flags: review?(anthony)
Attachment #8472013 - Flags: review?(kgrandon)
Attachment #8472013 - Flags: review?(anthony)
(Assignee)

Updated

3 years ago
Whiteboard: [planned-sprint c=3][in-sprint=v2.1-S1] → [planned-sprint c=6][in-sprint=v2.1-S1]
(Reporter)

Comment 39

3 years ago
Comment on attachment 8472013 [details] [diff] [review]
Create gaia-sim-picker component.

Review of attachment 8472013 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this. I'm pretty happy with the state of this. Let's get it landed so we can iterate. Thanks!

::: shared/elements/gaia_sim_picker/script.js
@@ +114,5 @@
> +    // XXX: Remove once bug 1040922 is fixed
> +    navigator.mozL10n.ready(function() {
> +      var children = this.shadowRoot.children;
> +      for (var i = 0; i < children.length; i++) {
> +        navigator.mozL10n.translateFragment(children[i]);

Interesting, does this not work on nested children?

@@ +140,5 @@
> +
> +  var stylesheet = baseurl + '../gaia_menu/style.css';
> +
> +  // TODO: We should make this a template node and use template.cloneNode, but
> +  // the nested gaia-menu element doesn't get initialized for some reason when

You might want to update this comment to say something along the lines that nested web components in templates do not yet work.
Attachment #8472013 - Flags: review?(kgrandon) → review+
(Assignee)

Updated

3 years ago
Whiteboard: [planned-sprint c=6][in-sprint=v2.1-S1] → [planned-sprint c=6][in-sprint=v2.1-S2]
Target Milestone: 2.1 S2 (15aug) → 2.1 S4 (12sep)
(Assignee)

Comment 40

3 years ago
Created attachment 8485609 [details] [diff] [review]
Create gaia-sim-picker component.

Updated PR.

Rebased, with a few small changes to support differences in APIs and bug fixes since this was posted.
Attachment #8472013 - Attachment is obsolete: true
Attachment #8472013 - Flags: review?(anthony)
Attachment #8485609 - Flags: review?(anthony)
I did a first high-level overview of this, will come back to it tomorrow.

My first comments:
- I don't think we need to separate gaia_sim_picker_test.js in two. This will create confusion as to where put the tests later I believe. (and that solves the duplication of a lot of code)
- long-tap-indicator.png is not needed afaict
- element_mocks_helper.js could be confusing. CustomElementsHelper? WCElementsHelper?
- stubs look like a duplication of mocks which would bitrot quickly I believe. I don't yet have suggestions on how to fix this.
(Assignee)

Updated

3 years ago
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
Comment on attachment 8485609 [details] [diff] [review]
Create gaia-sim-picker component.

Review of attachment 8485609 [details] [diff] [review]:
-----------------------------------------------------------------

This is not an r+ because of my previous comments.

Otherwise it looks good.

::: shared/elements/gaia_sim_picker/script.js
@@ +1,2 @@
> +'use strict';
> +/* global ComponentUtils */

Need to add LazyLoader

@@ +31,5 @@
> +
> +  proto._simSelectedCallbacks = [];
> +
> +  proto._dispatchSimSelected = function(cardIndex) {
> +    if (this.dispatchEvent(new CustomEvent(

We should store an intermediate variable for better reading.

::: shared/elements/gaia_sim_picker/style.css
@@ +9,5 @@
> +}
> +
> +/* FIXME/drs: For some reason, buttons aren't lining up correctly without these
> + * styles. I have no idea why. Even with them, they're aligned to the top of the
> + * menu instead of the bottom. */

Can we open a bug to fix gaia-menu?
Attachment #8485609 - Flags: review?(anthony)
Oh and as I told you on IRC, the example is not working because LazyLoader is missing.
(Assignee)

Comment 44

3 years ago
Created attachment 8493471 [details] [diff] [review]
Create gaia-sim-picker component.

(In reply to Anthony Ricaud (:rik) from comment #42)
> ::: shared/elements/gaia_sim_picker/style.css
> @@ +9,5 @@
> > +}
> > +
> > +/* FIXME/drs: For some reason, buttons aren't lining up correctly without these
> > + * styles. I have no idea why. Even with them, they're aligned to the top of the
> > + * menu instead of the bottom. */
> 
> Can we open a bug to fix gaia-menu?

This was fixed after comment 34. I accidentally squashed the removal of it into bug 1045820.

(In reply to Anthony Ricaud (:rik) from comment #43)
> Oh and as I told you on IRC, the example is not working because LazyLoader
> is missing.

Should be fixed now.

(In reply to Anthony Ricaud (:rik) from comment #41)
> - I don't think we need to separate gaia_sim_picker_test.js in two. This
> will create confusion as to where put the tests later I believe. (and that
> solves the duplication of a lot of code)

Done.

> - long-tap-indicator.png is not needed afaict

It is. Thanks for mentioning this, though. I missed a spot that this should have been used.

> - element_mocks_helper.js could be confusing. CustomElementsHelper?
> WCElementsHelper?

I went with CustomElementsHelper.

> - stubs look like a duplication of mocks which would bitrot quickly I
> believe. I don't yet have suggestions on how to fix this.

Yeah, agreed. I'm using the shared unit test mocks now.

I'm posting an updated patch for bug 1045820 after this.
Attachment #8485609 - Attachment is obsolete: true
Attachment #8493471 - Flags: review?(anthony)
Comment on attachment 8493471 [details] [diff] [review]
Create gaia-sim-picker component.

Review of attachment 8493471 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe we could add some docs at the top custom_elements_helper.js to help people know how to use it. Doesn't have to be done in this bug though.

r- thought because we also need to add gaia_sim_picker.{locale}.properties in all applications that were using it before. Right now, we can't see the SIM 1/SIM 2 label on multi sim action buttons.
Attachment #8493471 - Flags: review?(anthony) → review-
(Assignee)

Comment 46

3 years ago
Created attachment 8494590 [details] [diff] [review]
Create gaia-sim-picker component.

Good catch, thanks. I fixed this in bug 1045820 but we will not be landing them together, so that's important.

Updated PR.
Attachment #8493471 - Attachment is obsolete: true
Attachment #8494590 - Flags: review?(anthony)

Updated

3 years ago
Attachment #8494590 - Flags: review?(anthony) → review+
(Assignee)

Comment 47

3 years ago
https://github.com/mozilla-b2g/gaia/commit/2f7e36df28bb47461b7362df84d5a3ec0ed8429a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
> gaia-sim-picker-button=SIM{{ n }}

Aren't we using a space between "SIM" and "n" across Gaia? Change of plan or mistake?
You need to log in before you can comment on or make changes to this bug.