1100779 - Create SwipeableLayoutPageView component

Status: RESOLVED FIXED

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

Description

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

For emoji layout spec, we should have a swipeable page view for each category of emoji, like 'People', 'Nature', etc., see http://emojipedia.org/.

Comment 1

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Patch V1

Tim, 

This is just a WIP to present a pretty basic prototype for emoji function, hope to get early feedback from you.

ToDo: 
 1. No page indicator for the swipeable area.
 2. Need to fine tune the swipe gesture, like to consider the velocity of the swipe not just the distance.
 3. The UI style for bottom row is not complete.
 4. Need to get feedback from UX about the ordering of the emoji we need to show.

Thanks.

Comment 2

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8555789 [details] [review]
Patch V1

Patch updated and I think this is ready for 1st phase ui review.

Omega, please help give it a try with my patch, and give some feedbacks if this works for you.

You would need to add 'emoji' layout to the following build command.
GAIA_KEYBOARD_LAYOUTS=en,pt-BR,es,de,fr,fr-CA,pl,ko,zh-Hans-Pinyin,en-Dvorak,emoji make install-gaia

Please note that the emoji sets (and ordering) is not firmed, so please help focus on the interactions like switching emoji categories, swiping through the emojis, etc.
The "Recent" emojis are not implemented yet.


For some visual details
-------------------------
Did not ask for visual review, due to the following reasons,
 1. I used Android emoji font to do the testing, so the padding between the emoji icons might need some tweaks after we get our version of emoji font.

 2. The bottom row is not implemented as spec, will need to clarify the requirements with Carol.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8555789 [details] [review]
Patch V1

Great progress! I however think it's important to seperate the idea of Emoji layout with SwipeableLayoutPageView so we could generalize these two.

I haven't try the patch, it's reflow/repaint/fps etc. We need to make sure Gecko paints a section one at a time instead of paints a small strip of pixels at a frame.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #3)
> Comment on attachment 8555789 [details] [review]
> WIP
> 
> Great progress! I however think it's important to seperate the idea of Emoji
> layout with SwipeableLayoutPageView so we could generalize these two.

That also means remove all reference in class names etc with the word "emoji" from SwipeableLayoutPageView.

> I haven't try the patch, it's reflow/repaint/fps etc. We need to make sure
> Gecko paints a section one at a time instead of paints a small strip of
> pixels at a frame.

Comment 5

[Omega Feng [:Omega] [:馮於懋]]

Comment on attachment 8555789 [details] [review]
Patch V1

Nice WIP! As discussion offline, please fine tune the swipe interaction (like swipe cancellation, etc.), consider vibration timing, and tapping visual hint. Thanks a lot!

Comment 6

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8555789 [details] [review]
Patch V1

Tim,

Patch updated, hope this time I am going in the right direction.
Could you please help take a look and give me some advices?

Now, SwipeablePageView should be a general view class to render a "SwipeablePanel" and some switching keys.
(The SwipeablePanel was separated out in my previous attempt to address the feedback comments, and I think we could keep it as is).

I created EmojiSectionView to render the "emoji section" that would be embedded in the SwipeablePanel, hope this makes sense.

For the emoji data in layout def. file, I tend to put a simple array of chars in it, so that it would be easier to be modified by contributors or designers.

Thank you for your patience.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8555789 [details] [review]
Patch V1

I explicit said, please move all Emoji specific parts to EmojiKeyView.
I don't see why SwipableSectionView and SwipeablePageView can't be generalized.

Please correct them.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8555789 [details] [review]
Patch V1

Actually, I should give feedback+ because the division of modules looks good.

We just need to sort out the naming and the deviation* right.

* if (...) { do emoji specific thing } else { do usual things }

Comment 9

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8555789 [details] [review]
Patch V1

Tim,

I have refined the code again as you advised, and also added unit tests.
Could you please help review it?

Thanks.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8555789 [details] [review]
Patch V1

Did you verify the re-paint?

Comment 11

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Yes, from my test, the repaint would occur only once for a section, so it should be good.
Thanks.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8555789 [details] [review]
Patch V1

I have serious doubt/trouble understand the detail here.

https://github.com/mozilla-b2g/gaia/pull/27742/files#r24653771

Let's talk in person if you think your idea can't be expressed in code.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://github.com/mozilla-b2g/gaia/pull/27742#discussion_r24653771

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8555789 [details] [review]
Patch V1

Tim, thanks for the for the offline discussion and here is the update to remove SwipeableSectionView and move it into SwipeablePanelView as a _renderSection() function.

For the changes after the discussion, you may refer to this commit,
https://github.com/RudyLu/gaia/commit/74573c3fcf1be950e8e73da1a5266a36ada584c4

Comment 15

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/f6e480fb8f7ce6b23c485106eaf0f48bb19c8d9f

Thanks for the review!