1055897 - [settings] add settings dialog support

Status: RESOLVED FIXED

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

Description

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

In this bug, I'll try to implement the first prototype of settings dialog to make sure it is aligned with UX.

Comment 1

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on master

Comment 2

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi Jenny,

we are starting to think the prototype of Settings Dialog.

Is there any UX spec with needed UI, animations, plan ... etc for this feature ? thanks

Comment 3

[Jenny Lee]

Hi EJ, per discussion, please check out building block guideline for what you need. Thanks!

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8475621 [details] [review]
patch on master

Arthur, I am sure this feature would take longer time to get r+, let me put the f? flag on you first and make sure the direction is right.

Thanks :)

Comment 5

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8475621 [details] [review]
patch on master

Overall it looks good. But we need to take the cases we discussed offline into consideration. And we should also investigate the feasibility of leverage gaia components.

Comment 6

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8475621 [details] [review]
patch on master

Codes, jsdocs and tests are all finished, so I think this patch is ready to be reviewed.

Arthur, please help me review this patch when you have time, thanks !

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8475621 [details] [review]
patch on master

Good work!! There are just a few comments and they should be easy to address. Please check them in github, thanks.

Comment 8

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8475621 [details] [review]
patch on master

Arthur, just addressed all comments and fixed tests.

Please help me review again when you aren't so busy. Thanks !

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8475621 [details] [review]
patch on master

There are a few comments to be addressed before merging. Please check them in github. And we should also fix the animation issue as what we discussed. Thanks!

Comment 10

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8475621 [details] [review]
patch on master

Arthur, can you give it a check and give me some feedbacks about that ? thanks :)

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

The patch looks good! Note that we need transitions of fading for dialogs and no transitions for confirmations.

Comment 12

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8475621 [details] [review]
patch on master

Arthur, forgot to set ni? on you xD, I already updated the transition to fade and it works perfect, can you check it to see whether we are on the right way !? THanks :P

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

The code is good with one last comment addressed.

Comment 14

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8475621 [details] [review]
patch on master

Arthur, if this patch is finalized, I'll try to remove all test-used codes in this patch and will squash them after r+. After that, we can start to list follow-up bugs in its dependency and try to change all of them to real dialog in Settings app !

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8475621 [details] [review]
patch on master

Awesome! r=me, please remember to remove the code for testing. Let's start to switch to the new dialogs!

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Merged into Gaia/master (2.2): https://github.com/mozilla-b2g/gaia/commit/2196e28c907b7fcae37baed509622f242745ff00

Thanks Arthur !!

Comment 17

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

Ej, this API is breaking the programming patterns for localizable API's established in https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Writing_APIs_that_operate_on_L10nIDs

It should always expect to get l10nId/l10nArgs instead of strings.

Do you want me to reopen this bug or file a follow up? I'd like to get this fixed before we branch 2.2 and/or start using DialogService with its current API more.

Comment 18

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

ok, I did read through the l10n best practices and I think it's okay to do some refactor works on it although this would make DialogService API not so intuitive to be honest.

I'll discuss with Arthur about this case and I already filed a bug 1111903 for this, thanks.

Comment 19

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

In our API, we can pass `message` and `title` to make sure DialogService to render these strings on UI. But by following our l10n best practices, in worst case, we have to pass 4 extra parameters like this : 

```
DialogService.alert({
  titleL10nId: 'ThisIsTitleId',
  titleL10nArgs: {},
  messageL10nId: 'ThisIsMessageId',
  messageL10nArgs: {}
})
```

To be honest, I don't think this makes sense for API design, any better idea, guys ?

Can we just leave this logic on caller ? For example, caller has to create a pseudo element and make sure these information would be rendered correctly on that element and then, we can directly retrieve `textContent` from the DOM and pass it to API ? (Just an idea, for me, this is not a good idea either hmmm)

Comment 20

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

moved the conversation to bug 1111903