Closed Bug 1055897 Opened 10 years ago Closed 10 years ago

[settings] add settings dialog support

Categories

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

x86
macOS
defect
Not set
normal

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: 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)
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)
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)
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)
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+
Status: NEW → RESOLVED
Closed: 10 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)
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: