If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[settings] add settings dialog support

RESOLVED FIXED

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: eragonj, Assigned: eragonj)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:+)

Details

Attachments

(1 attachment)

In this bug, I'll try to implement the first prototype of settings dialog to make sure it is aligned with UX.
Created attachment 8475621 [details] [review]
patch on master
Assignee: nobody → ejchen
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
Flags: needinfo?(jelee)

Comment 3

3 years ago
Hi EJ, per discussion, please check out building block guideline for what you need. Thanks!
Flags: needinfo?(jelee)
Summary: [settings] add settings panel support → [settings] add settings dialog support
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 :)
Attachment #8475621 - Flags: feedback?(arthur.chen)
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.
Attachment #8475621 - Flags: feedback?(arthur.chen) → feedback+
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 !
Attachment #8475621 - Flags: review?(arthur.chen)
tracking-b2g: --- → +
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.
Attachment #8475621 - Flags: review?(arthur.chen)
Blocks: 1097451
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 !
Attachment #8475621 - Flags: review?(arthur.chen)
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!
Attachment #8475621 - Flags: review?(arthur.chen)
Comment on attachment 8475621 [details] [review]
patch on master

Arthur, can you give it a check and give me some feedbacks about that ? thanks :)
Flags: needinfo?(arthur.chen)
The patch looks good! Note that we need transitions of fading for dialogs and no transitions for confirmations.
Flags: needinfo?(arthur.chen)
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
Flags: needinfo?(arthur.chen)
The code is good with one last comment addressed.
Flags: needinfo?(arthur.chen)
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 !
Attachment #8475621 - Flags: review?(arthur.chen)
Blocks: 1103801
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!
Attachment #8475621 - Flags: review?(arthur.chen) → review+
Merged into Gaia/master (2.2): https://github.com/mozilla-b2g/gaia/commit/2196e28c907b7fcae37baed509622f242745ff00

Thanks Arthur !!
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(ejchen)
Depends on: 1111903
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.
Flags: needinfo?(ejchen)
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)
moved the conversation to bug 1111903
Depends on: 1166495
You need to log in before you can comment on or make changes to this bug.