Review: Needs Fixing * Please do not use tabs for indentation in Python code (your edition of the `sc_list` method introduced tabs).
* Some statements are not terminated with a semicolon (chrome.js:869, chrome.js:876) * Avoid trailing commas (in object literals and arrays), chrome.js:893 some browsers will error out on this * Why is the store for shortcuts called "sc_list" yet is initialized to an object? Also when removing shortcuts, you're passing in an empty object to a template parameter taking an Array a few lines above... * Why have you added `active_menu_name` and `active_id` as session attributes? * I believe shortcuts store menu actions, is that correct? In that case, why does creating shortcut not fall in the purview of the ActionManager or the ViewManagerAction? I'm not sure it makes sense to add it in the SearchView since (as opposed to custom filters) it has little to do with searching. Am I wrong? * Avoid using hardcoded IDs (even for CSS) as they impede reuse and the ability to instantiate objets multiple times (it's unlikely that would be needed for headers, but you never know). Instead, please use classes with a `oe-` prefix (so `#shortcuts` should be called `.oe-shortcuts`, and the CSS rules should probably be prefixed with `.openerp`) * Why not load header shortcuts as part of the header-loading process, so the header is "ready" only when shortcuts are loaded? * In the template, you use a bare `a` element (with no attributes). In many browser, a link with no `@href` will not be recognized as a link, and may not get styled correctly. Also, indentation is incorrect (`li` is indented 8 spaces from its parent `ul`) * Why re-render the shortcuts in full in order to "remove" them? Why not empty() the whole thing, or just remove the `li` elements? Also why the need for a separate method which is only called once from `on_logout`? It's not like the naming makes it clear *what* is removed (as opposed to the `shortcut_`-prefixed methods above). I think `Header.remove` should just be inlined into `Header.on_logout`. * I don't think you should create a new actionmanager to execute the shortcut action. Why not handle it the same way `Menu` does, just call a placeholder bindable method and let the WebClient bind itself to that method and handle the action through its own actionmanager? * Don't use an attribute selector `[$attr=$somevalue]` for ids, use `#`: `#` will always be special-cased for efficient lookups (and is shorter), `[id=]` may not be. * In CSS, don't use relative paths to files (images & al). Use absolute pats (from `/base`) instead. * Why do you recreate the whole header every time you add or remove a shortcut? What if others have bound events on the old one? I don't think that's sensible * Also, you create the new header as an instance to the context (`this`) of the click event handler (search.js:371)… that would be the DOM element which was just clicked. I don't think that's very useful. -- https://code.launchpad.net/~openerp-dev/openerp-web/trunk-shortcut-vme/+merge/70721 Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openerp-web/trunk-shortcut-vme. _______________________________________________ Mailing list: https://launchpad.net/~openerp-dev-gtk Post to : [email protected] Unsubscribe : https://launchpad.net/~openerp-dev-gtk More help : https://help.launchpad.net/ListHelp
