+++ This bug was initially created as a clone of Bug #1074653 +++ This is the first small step for keyboard rendering refactoring. As KeyView is used for general key rendering and the key in AlternativeCharMenuView, we should create a KeyView component for this.
Comment on attachment 8518844 [details] [review] Patch V1 Try to separate KeyView out and let render.js and alternativeCharMenu be able to reuse it. Tests are not done yet in order to get early feedback first. Tim, could you please take a look? Thank you.
Comment on attachment 8518844 [details] [review] Patch V1 Nice try, but I see few flaws in this patch: 0. Make up your mind on file naming -- I don't see ***_view.js in alternatives_char_menu.js but I see key_view.js 1. I don't think the future LayoutView and the current AlternativesCharMenu should share the same KeyView object. Their DOM and available methods are different. I won't object it one could inherit another though. 2. It seems that you are not moving any methods in render.js to the view object? That's probably because of below: 3. render.js has no reference to the created KeyView instance. This patch should be on top of bug 1020847 and use the viewMap to find the instance. 4. I don't think we need getElement and a public |container| property is necessary. 5. You probably want to develop common interfaces of views here (or maybe even create a ViewBase) like render() etc.
Forget to mention AlternativesCharMenu has no access to the view instances it created either, in 3. Without 2 and 3, the view class is simply a custom template engine. :'(
Comment on attachment 8518844 [details] [review] Patch V1 Sorry for the delay as I took a while to look into the "target <-> DOM element" mapping and "target -> view" mapping. Patch update: -- 1. Renaming the view file to end with _view.js 2. Create render() function for creating the actual DOM element. 3. el property to return the DOM element. 4. Move highlight() and unHighlight() method to KeyView. 5. Also clean up the target <-> DOM mapping in alternativeCharMenuView. Tim, could you please give a glance on this? Thanks.
Comment on attachment 8518844 [details] [review] Patch V1 We are indeed moving toward the right direction. Thanks! One requirement I overlooked is https://github.com/mozilla-b2g/gaia/pull/25930/files#r20200376 and we need to discuss this in person.
Attachment #8518844 - Flags: feedback?(timdream) → feedback+
Patch updated to address the review comment. - Add registerView() method to IMERender for Views register "target -> view" mapping. - Remove target -> DOM element mapping - Clean up the code for set target <-> view mapping - Address other review comments. Will work on adding the tests.
Comment on attachment 8518844 [details] [review] Patch V1 I think this is ready for review. Tim, could you help review it again? Thank you.
Comment on attachment 8518844 [details] [review] Patch V1 I think this is good enough to land. Please make sure all test passes after you address the review comment. I think the time takes here will save us some time making arch decisions as we move through the cleaning up. Let's try to speed up the process in the next bugs. Thanks.
Attachment #8518844 - Flags: review?(timdream) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.