1103894 - [Settings][Dialog] Keyboard warning should be shown as a dialog

Status: RESOLVED FIXED

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

Description

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

The warning should be shown as a dialog. We should use settings dialog to show it.

STR:
1. Launch settings app
2. Keyboards > Select Keyboards
3. Un-check all number layouts or text layouts.

Actual:
  The warning slides from the right hand side.

Expected:
  The warning is displayed like a dialog

Comment 1

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

Attachment: link to https://github.com/mozilla-b2g/gaia/pull/26713

EJ, could you help review the patch? The changes to KeyboardContext is not trivial, feel free to pin me if you need clarification, thanks.

Comment 2

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

Comment on attachment 8534911 [details]
link to https://github.com/mozilla-b2g/gaia/pull/26713

Basically this patch looks good to me, but in order to clarify something, I left some comments on Github and please check it first.

Thanks Arthur ! Feel free to ping me back when you replied.

Comment 3

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

Hi Kevin,

I noticed that the title in gaia-confirm was limited to one line since bug 1061441 was landed. It is common to have multiple lines of messages in settings app and it may still true in other apps. Although we could override the style, but I was wondering which should be the default style. Could you shed some light on this? Thanks.

Comment 4

[Kevin Grandon :kgrandon]

(In reply to Arthur Chen [:arthurcc] from comment #3)
> Hi Kevin,
> 
> I noticed that the title in gaia-confirm was limited to one line since bug
> 1061441 was landed. It is common to have multiple lines of messages in
> settings app and it may still true in other apps. Although we could override
> the style, but I was wondering which should be the default style. Could you
> shed some light on this? Thanks.

If it's a valid use-case then we shouldn't override the style, we should change the gaia-confirm styling. Perhaps we can change the white-space property there? I wouldn't really know until trying. Seems like we should tackle that in another bug though?

Comment 5

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

Changing white-space works for my use case but it would also break the fix of bug 1061441. I'll override the style in settings app to avoid the regression. Let's discuss where should the change be made in a separate bug.

Comment 6

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

(In reply to Kevin Grandon :kgrandon from comment #4)
> If it's a valid use-case then we shouldn't override the style, we should
> change the gaia-confirm styling. Perhaps we can change the white-space
> property there? I wouldn't really know until trying. Seems like we should
> tackle that in another bug though?

Filed bug 1111494 tracking the issue.

Comment 7

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

Comment on attachment 8534911 [details]
link to https://github.com/mozilla-b2g/gaia/pull/26713

EJ, I've fix the truncation issue in settings app. Would you mind review the patch again? Thanks.

Comment 8

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

Comment on attachment 8534911 [details]
link to https://github.com/mozilla-b2g/gaia/pull/26713

Arthur, this patch looks good to me, r+.

BTW, I left some comments on github about Kevin has provided a solution for such case in gaia component and you are assigned as a reviewer now. Not sure whether you want to land yours first or Kevin's.

If yours got landed earlier, we should file another follow-up bug to remove these hacks we done in this bug and make sure that bug would block this one.

Comment 9

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

I've revised the patch based on Kevin's fix. Waiting for treeherder. Thanks, EJ.

Comment 10

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

(In reply to Arthur Chen [:arthurcc] from comment #9)
> I've revised the patch based on Kevin's fix. Waiting for treeherder. Thanks,
> EJ.

ok ! Arthur ++

Comment 11

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

new PR: https://github.com/mozilla-b2g/gaia/pull/26810

Comment 12

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

master: 4353228b849597408f79e16ed3ade7df02c6b45a