[Settings] Split up phone_lock.js

RESOLVED WONTFIX

Status

Firefox OS
Gaia
RESOLVED WONTFIX
5 years ago
4 years ago

People

(Reporter: kgrandon, Assigned: kgrandon)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
We should split up the phone_lock.js file so the code for the dialog is separate from the main phone lock panel.
(Assignee)

Comment 1

5 years ago
Created attachment 831164 [details] [review]
Github pull request
(Assignee)

Updated

5 years ago
Attachment #831164 - Attachment description: WIP - Github pull request → Github pull request
(Assignee)

Comment 2

5 years ago
Comment on attachment 831164 [details] [review]
Github pull request

Hey Arthur - could you tell me what you think of this one?

I needed a way to pass arguments to panels, and for now data attributes seem to be a fairly sane way to do this. In the future I think I'd like to move to that proposal I emailed you about - but this is probably the best option with the least amount of code changes.

In order to pass arguments around, I changed the currentPanel setter to a changePanel() method which takes a panel id and params object. Right now it's only using dataset on the panel section, but we can change this in the future to be a URL, or object instantiation for example.
Attachment #831164 - Flags: review?(arthur.chen)
Yes, I think we do need a way to pass arguments when changing panels. In addition to a params object, what do you think to have a callback function as the third parameter, so that the navigated panel can bring the results back?
(Assignee)

Comment 4

5 years ago
(In reply to Arthur Chen [:arthurcc] from comment #3)
> Yes, I think we do need a way to pass arguments when changing panels. In
> addition to a params object, what do you think to have a callback function
> as the third parameter, so that the navigated panel can bring the results
> back?

I would really rather avoid yet another callback from panel startup. I think passing the arguments into the panel during instantiation is the way to go.
Comment on attachment 831164 [details] [review]
Github pull request

Hi Kevin, sorry for the late review. 

I left some comments regarding the part of passing data to panels. wdyt?
Attachment #831164 - Flags: review?(arthur.chen)
(Assignee)

Comment 6

5 years ago
Created attachment 8335125 [details] [review]
Alternative pull request

Hi Arthur,

I agree that using data attributes for passing data to panels is rather fragile. I would much rather have something like this which has a defined interface for passing data into a panel. In this case the render method is called whenever the panel is shown, and it can take any necessary data passed into it.

This would be a migration that I would start doing for other panels, and the upcoming work to remove sub-panel dependencies.

I still need to do more testing, but wanted to get some initial feedback on this approach. Thanks!
Attachment #8335125 - Flags: feedback?(arthur.chen)
Comment on attachment 8335125 [details] [review]
Alternative pull request

Evelyn, any ideas?
Attachment #8335125 - Flags: feedback?(ehung)
(Assignee)

Comment 9

4 years ago
Comment on attachment 8335125 [details] [review]
Alternative pull request

Seems like we're going with a different approach now.
Attachment #8335125 - Flags: feedback?(ehung)
Attachment #8335125 - Flags: feedback?(arthur.chen)

Updated

4 years ago
Keywords: perf
Whiteboard: [c= p=3 s= u=]
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.