973827 - Power button modal should be an object menu instead of a custom implementation

Status: RESOLVED FIXED

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

Description

[Victoria Gerchinhoren [:vicky]]

Attachment: power_button_modal.png

Power button triggers a set of actions, that's why it should be a different compoenent, Action menu instead of value selector.

Comment 1

[Joan Leon [:joan]]

Attachment: Power_button_modal_Action_menu.png

Comment 2

[Joan Leon [:joan]]

Attachment: Power button modal

Comment 3

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8394019 [details]
Power button modal

>https://github.com/nucliweb/gaia/compare/master...973827-Power-button-modal

Comment 4

[Joan Leon [:joan]]

Comment on attachment 8394019 [details]
Power button modal

https://github.com/mozilla-b2g/gaia/pull/17386

Comment 5

[Joan Leon [:joan]]

Attachment: Power-button-modal

Comment 6

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8394160 [details] [review]
Power-button-modal

CSS LGTM. Asking module owners for JS review.

Comment 7

[Joan Leon [:joan]]

commit 3ba39ebd3fd4613640f9477898a008580ef8fb41

Comment 8

[Anthony Ricaud (:rik)]

Back out: https://github.com/mozilla-b2g/gaia/commit/072a6683cda997a2b6c3954d6616461bdc342b77

I had to revert this because of several issues:
- The commit message had no bug number in it. Please format your messages like this "Bug xxx - Solving foobar"
- Neither the commit or the merge commit had any reviewer information. One of them should contain "r=arnau,alive" or "r=arnau r=alive"
- The linter tests were failing.
- A lot of System ActionMenu unit tests were failing.
- test_power_button_long_press.TestPowerButton.test_power_button_long_press was failing.

Joan: You need to wait for a green travis before landing your patches. Given all the issues with that patch and the fact that none of your previous patches included bug numbers nor reviewers, I've revoked your Github rights on Gaia. For your next patches, you'll need to ask someone to land patches for you or put "checkin-needed" in the keyword field. Once you have more patches and experience on the project, we can give you that permission back.

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

Sorry I didn't notice the commit message when reviewing and test results, I will pay attention next time.

Comment 10

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

In favor of Joan, I would say he is an expert in CSS and not in Javascript, neither am I. That's why I added an app owner to further review his code.
I will me reviewing his work in the following weeks in order to get his permissions back.
Thank you guys.

Comment 11

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Attachment: patch in github

Comment 12

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Attachment: after_patch.png

Comment 13

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8433487 [details] [review]
patch in github

Alive, could you review that patch?
I've retaken what Joan did in the past with a different approach to avoid previous test failures.
Thanks

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8433487 [details] [review]
patch in github

test_power_button_long_press.py test_power_button_long_press.TestPowerButton.test_power_button_long_press
is broken
could you get it over?

Comment 15

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8433487 [details] [review]
patch in github

Could you please review it again?
I've fixed the test :)

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8433487 [details] [review]
patch in github

Everything is fine except the change to testvar-template :)

Comment 17

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Merged: a06fc5812b4cb633d20992913a05bdb3f64c9622