Closed Bug 1100779 Opened 6 years ago Closed 6 years ago

Create SwipeableLayoutPageView component

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-master fixed)

RESOLVED FIXED
Tracking Status
b2g-master --- fixed

People

(Reporter: rudyl, Assigned: rudyl)

References

Details

Attachments

(1 file)

46 bytes, text/x-github-pull-request
timdream
: review+
timdream
: feedback+
Details | Review
For emoji layout spec, we should have a swipeable page view for each category of emoji, like 'People', 'Nature', etc., see http://emojipedia.org/.
Status: NEW → ASSIGNED
Assignee: nobody → rlu
Attached file 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.
Attachment #8555789 - Flags: feedback?(timdream)
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.
Attachment #8555789 - Flags: ui-review?(ofeng)
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.
Attachment #8555789 - Flags: feedback?(timdream) → feedback+
(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 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!
Attachment #8555789 - Flags: ui-review?(ofeng)
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.
Attachment #8555789 - Flags: feedback+ → feedback?(timdream)
Attachment #8555789 - Attachment description: WIP → WIP 2
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.
Attachment #8555789 - Flags: feedback?(timdream) → feedback-
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 }
Attachment #8555789 - Flags: feedback- → feedback+
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.
Attachment #8555789 - Attachment description: WIP 2 → Patch V1
Attachment #8555789 - Flags: review?(timdream)
Comment on attachment 8555789 [details] [review]
Patch V1

Did you verify the re-paint?
Flags: needinfo?(rlu)
Attachment #8555789 - Flags: review?(timdream)
Yes, from my test, the repaint would occur only once for a section, so it should be good.
Thanks.
Flags: needinfo?(rlu)
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.
Flags: needinfo?(rlu)
Attachment #8555789 - Flags: review?(timdream) → review-
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
Flags: needinfo?(rlu)
Attachment #8555789 - Flags: review- → review?(timdream)
Attachment #8555789 - Flags: review?(timdream) → review+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.