cordova-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <>
Subject [jira] [Commented] (CB-5138) Code review changes for the StatusBar plugin
Date Thu, 20 Mar 2014 01:10:43 GMT


ASF GitHub Bot commented on CB-5138:

Github user shazron commented on the pull request:
    Thanks. Be aware however that this will change in the future, see this code review feedback:

> Code review changes for the StatusBar plugin
> --------------------------------------------
>                 Key: CB-5138
>                 URL:
>             Project: Apache Cordova
>          Issue Type: Improvement
>          Components: Plugin Statusbar
>    Affects Versions: Master
>            Reporter: Shazron Abdullah
>              Labels: statusbar-plugin
> From Andrew Grieve in the ML:
> -----
> Looking at the Keyboard & Statusbar plugins, would you be opposed to using:
> <clobbers target="cordova.plugins.statusBar" />
> instead of:
> <clobbers target="window.StatusBar" />
> Would require a major version bump, but probably it has little usage since
> it's new & in labs still?
> Thinking here is that since it's not implementing any spec, we should keep
> symbols off of window / navigator objects. Would be good to encourage
> plugins not to pollute the global namespace I think.
> Other points from code-review standpoint:
> - Why make StatusBar a function? It has no prototype methods.
> - You're calling exec() from the module scope. You should add a <runs/> to
> the <js-module> to ensure it gets run at start-up. You should also delay
> deviceready until it executes. cordova-plugin-device has an example of
> this.
> - Might be nicer to make the status-bar state a callback with
> keepCallback=true so that the native code is agnostic to where the
> namespace is mapped.

This message was sent by Atlassian JIRA

View raw message