1001391 - [Building Blocks] update date and time selectors to show larger font on selected items

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

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

Attachment: 05_OWD_ValueSelector_DatePicker.jpg

Comment 1

[Paco Rampas [:paco]]

Attachment: patch in github

Comment 2

[Paco Rampas [:paco]]

Attachment: datepicker.png

Comment 3

[Etienne Segonzac (:etienne)]

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

note: please use |camelCase| variable names

Having a _selected storing the last _currentIndex is a bit heavy handed and could lead to confusion.

I think we should toggle a dataset in setSelectedIndex (or a css class if Arnau prefers). Also we'll need to add a quick test in value_picket_test.js :)

I'm adding Arnau as a reviewer for the building blocks part.

Comment 4

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

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

Minor comments in GH for the CSS part.
r+ when fixed.
Awesome to see a small patch like that could make a huge impact on the UI perception :)

Comment 5

[Paco Rampas [:paco]]

> Having a _selected storing the last _currentIndex is a bit heavy handed and
> could lead to confusion.
Ok. Now we don't need _selected because the change of selected class is done on setSelectedIndex.

Also we'll need to add a quick test in value_picket_test.js
> :)
It is done.

Comment 6

[Paco Rampas [:paco]]

> Minor comments in GH for the CSS part.
> r+ when fixed.
> Awesome to see a small patch like that could make a huge impact on the UI
> perception :)
Great :D

Comment 7

[Etienne Segonzac (:etienne)]

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

It's shaping up :)

* element_children should be elementChildren, camel case instead of underscores
* we need a test asserting that the class is added to the new selected item and removed from the previous one

Comment 8

[Paco Rampas [:paco]]

> * element_children should be elementChildren, camel case instead of
> underscores
Done

> * we need a test asserting that the class is added to the new selected item
> and removed from the previous one
We have changed the test. Could you see it on github?

Comment 9

[Etienne Segonzac (:etienne)]

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

The system code changes are a-ok, so we're ready on this front.

But I played a bit with this patch on device, and the drop in performance is pretty significant.
The amount of reflows caused by the font size makes the picker really hard to use on a buri-level device.

Looks like having a transform: scale(1) on the .selected class and a transform: scale(0.8) otherwise is helping. Is there anything preventing us from using this approach?

Comment 10

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

Make sense :)
But while checking it with Paco we've seen text is moving a bit when using transform, so I think we should remove the scaling transition to improve performance.

Comment 11

[Paco Rampas [:paco]]

Hello Etienne;
I have updated the patch, could you see it?

I don't know why but there is a problem. The issue is that the first iteration (when the date picker prompt is opened the first time) the selected class don't work well. The opacity is always rendered ok but not the font-size. The result is that the selected data are with opacity 1, but not with font-size 2.2 (have 1.8 like without selected class). 

It is occurs only in the first iteration, therefore I think that it is a problem of rendering css, not of the class. But I don't know why.

Comment 12

[Etienne Segonzac (:etienne)]

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

The performance is much better!

I think your last issue comes from the fact that we don't clean the .selected class properly when going out of the picker (but we loose the _currentIndex so the previously selected item stays selected).

Comment 13

[Paco Rampas [:paco]]

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

Hello Etienne, 

I have worked in this patch and now the _currentIndex is ok. But, we have the same problem with the first interaction (doesn't render well the propiety font-size of .selected). Then, I use transform: scale() for changing the size of font and currently works ok always. But, only selecting month some times the months seems like to be broken (I don't know how explain it), but I think this is happening for using transform: scale().

Therefore, both solutions have problems but, from my point of view, seem that the issue is happening for others reasons of the system, not of the code of data picker.

Comment 14

[Etienne Segonzac (:etienne)]

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

Didn't encounter any issue :)
I think we should go with this patch.

Comment 15

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

Merged: e8a10acecd5c573b752882d987000d91cfd4382f