Closed
Bug 1055897
Opened 10 years ago
Closed 10 years ago
[settings] add settings dialog support
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Tracking
(tracking-b2g:+)
RESOLVED
FIXED
tracking-b2g | + |
People
(Reporter: eragonj, Assigned: eragonj)
References
Details
Attachments
(1 file)
In this bug, I'll try to implement the first prototype of settings dialog to make sure it is aligned with UX.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ejchen
Assignee | ||
Comment 2•10 years ago
|
||
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)
Hi EJ, per discussion, please check out building block guideline for what you need. Thanks!
Flags: needinfo?(jelee)
Updated•10 years ago
|
Summary: [settings] add settings panel support → [settings] add settings dialog support
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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)
Updated•10 years ago
|
tracking-b2g:
--- → +
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
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)
Comment 11•10 years ago
|
||
The patch looks good! Note that we need transitions of fading for dialogs and no transitions for confirmations.
Flags: needinfo?(arthur.chen)
Assignee | ||
Comment 12•10 years ago
|
||
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)
Comment 13•10 years ago
|
||
The code is good with one last comment addressed.
Flags: needinfo?(arthur.chen)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Blocks: settings-dialog
Comment 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
Merged into Gaia/master (2.2): https://github.com/mozilla-b2g/gaia/commit/2196e28c907b7fcae37baed509622f242745ff00
Thanks Arthur !!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 17•10 years ago
|
||
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)
Assignee | ||
Comment 18•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(ejchen)
Assignee | ||
Comment 19•10 years ago
|
||
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•10 years ago
|
||
moved the conversation to bug 1111903
You need to log in
before you can comment on or make changes to this bug.
Description
•