Closed Bug 1001391 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Gaia, defect)

x86
macOS
defect
Not set

Tracking

(b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S3 (6june)
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: arnau, Assigned: paco)

Details

Attachments

(2 files, 1 obsolete file)

Attached image 05_OWD_ValueSelector_DatePicker.jpg (obsolete) —
No description provided.
Assignee: nobody → pacorampas
Attached file patch in github
Attachment #8417879 - Flags: review?(etienne)
Attached image datepicker.png
Attachment #8412580 - Attachment is obsolete: true
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.
Attachment #8417879 - Flags: review?(etienne)
Attachment #8417879 - Flags: review?(arnau)
Attachment #8417879 - Flags: feedback-
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 :)
Attachment #8417879 - Flags: review?(arnau) → review+
Attachment #8417879 - Flags: feedback- → feedback?(etienne)
> 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.
> 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 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
Attachment #8417879 - Flags: feedback?(etienne) → feedback-
Attachment #8417879 - Flags: feedback- → feedback?(etienne)
> * 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 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?
Attachment #8417879 - Flags: feedback?(etienne)
Flags: needinfo?(arnau)
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.
Flags: needinfo?(arnau)
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.
Attachment #8417879 - Flags: review+ → review?(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).
Attachment #8417879 - Flags: review?(etienne)
Attachment #8417879 - Flags: review?(etienne)
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 on attachment 8417879 [details] [review]
patch in github

Didn't encounter any issue :)
I think we should go with this patch.
Attachment #8417879 - Flags: review?(etienne) → review+
Merged: e8a10acecd5c573b752882d987000d91cfd4382f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S3 (6june)
You need to log in before you can comment on or make changes to this bug.