991062 - Call log: Take out filters while in edit mode

Status: VERIFIED FIXED

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

Description

[Victoria Gerchinhoren [:vicky]]

Attachment: Shows current implementation.

Filters should not be present while in call logs edit mode, the possible actions are selecting, unselecting and removing items. Having the filters there would only cause interaction noise.

Comment 1

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

Attachment: patch in github

Comment 2

[Jose Manuel Cantera]

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

thanks Arnau, f+ by this should be r+ by a dialer peer, thus, redirecting r? to :Rik

Comment 3

[Anthony Ricaud (:rik)]

That makes sense to me but just confirming with Carrie that this is ok. See comment 0.

Comment 4

[Anthony Ricaud (:rik)]

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

Why the transform? Isn't .recents-edit .recents-filter-container { display: none } working?

Using display: none is also better for accessibility.

Comment 5

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

:Rik As far as I can remember, I first tried this but the transition from one stage to the other looked weird. I'll create a new patch tomorrow in order to compare it.

Comment 6

[Anthony Ricaud (:rik)]

Which transition? I believe I've removed all of them because they were not visible on a device and slowed down the operation.

Comment 7

[Carrie Wang [New account: [e-mail]]]

(In reply to Anthony Ricaud (:rik) from comment #3)
> That makes sense to me but just confirming with Carrie that this is ok. See
> comment 0.

Yes, this make sense to me. Thanks! guys.

Comment 8

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

(In reply to Anthony Ricaud (:rik) from comment #6)
> Which transition? I believe I've removed all of them because they were not
> visible on a device and slowed down the operation.

Please review the new patch without transitions.
I agree in low end devices the transition won't show, and may slow down the action, although transform animations are not much expensive.
My point is trying to provide a richer experience to capable devices.
But if that works for you, works for me ;)

Comment 9

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

Attachment: patch in github

Comment 10

[Anthony Ricaud (:rik)]

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

I'd love to keep the transitions but with a lot of entries, we were not seeing anything.

Sorry I forgot to say so the first time but we also need to remove the specific code that was introduced in https://github.com/mozilla-b2g/gaia/commit/78f1497046f792fc4ac474e43f8d418ce4f6af93. We should keep the new tests but remove the code that disables edit mode when clicking a filter.

Comment 11

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! I was helping Arnau on this bug regarding comment 10 and found an issue I do not know if you are currently aware of.

The point is that I did not want just to delete the unit tests related to exiting the edit mode when filtering the calls which no longer apply, but to include an integration test which validated that after entering the edit mode, no filtering options are provided to the user. You can find my updates added to Arnau's ones here: https://github.com/gtorodelvalle/gaia/compare/rnown-call-log-bug-991062?expand=1 The new integration tests can be found at apps/communications/dialer/test/marionette/call_log_test.js

The problem is that I realised that the Call Log is not starting correctly when running the integration tests and consequently the new provided integration test fails. You can check this by pulling my branch (https://github.com/gtorodelvalle/gaia/tree/rnown-call-log-bug-991062) and running:

make test-integration TEST_FILES=apps/communications/dialer/test/marionette/call_log_test.js 

The B2G Desktop will pop up running the new integration test which just opens the Dialer app and taps on the Call Log tab. As you will see the Call Log is not loading completely since no "No calls recorded..." message is shown. If fact, if you click on the "Missed" tab you will notice that nothing happens. On the other hand, you can click on the "Contacts" app to go to it and everything works as expected.

I will contact Arnau and Anthony via IRC tomorrow to decide the way to proceed ;-)

Comment 12

[Germán Toro del Valle (:gtorodelvalle)]

In fact and since it may take some time for Anthony to try what I mention in comment 11 I think I'll better need-info him :-)

Anthony, are you aware of the issue regarding the loading of the Call Log when running the communications app in the B2G Desktop to run integration tests, for example? (please have a look and try what I mention in comment 11).

Feel free to contact me via IRC ;) Thanks!

Comment 13

[Anthony Ricaud (:rik)]

I'm not surprised. It says navigator.mozIccManager is undefined and that's expected on a desktop :) .

We mostly write unit tests for Dialer until we get a proper way to effectively do integration. For this patch, it looks like testing that we set/unset .recents-edit is the proper thing to do.

Comment 14

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 18972.html

According to Anthony's comment 13, I have skipped the integration test created to validate that the filters are hidden when the edit mode is activated until it is possible to start the Call Log using the B2G Desktop.

Apart from this, the PR is ready for review. Thanks!

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8417874 [details]
18972.html

r=me with an optional comment on github

Comment 16

[Germán Toro del Valle (:gtorodelvalle)]

Minor counter-comment :p in the PR ;-) As soon as we agree on this, I'll merge it ;)

Comment 17

[Germán Toro del Valle (:gtorodelvalle)]

I have just cross check it with UX and  https://www.dropbox.com/s/jqnxwn21jylg5cn/call-log_no_filters_using_display.png is preferred instead of https://www.dropbox.com/s/on94bqt2k90csea/call-log_no_filters_using_visibility.png. So, merging ;-) Thanks Etienne!

Comment 18

[Germán Toro del Valle (:gtorodelvalle)]

Merged in master: https://github.com/gtorodelvalle/gaia/commit/647f369f881c6180515a1111fec36b9e98ed3dea

Comment 19

[Loli (:lolimartinezcr)]

Bug detected: https://bugzilla.mozilla.org/show_bug.cgi?id=1014495

Comment 20

[Loli (:lolimartinezcr)]

Tested and fine
Hamachi
2.0
Gecko-564e4e1
Gaia-a436da1