You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by li...@apache.org on 2008/03/01 15:57:58 UTC
svn commit: r632597 - in /incubator/shindig/trunk: config/syndicator.js
features/skins/skins.js
Author: lindner
Date: Sat Mar 1 06:57:57 2008
New Revision: 632597
URL: http://svn.apache.org/viewvc?rev=632597&view=rev
Log:
Implement skins feature as described in SHINDIG-98
Modified:
incubator/shindig/trunk/config/syndicator.js
incubator/shindig/trunk/features/skins/skins.js
Modified: incubator/shindig/trunk/config/syndicator.js
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/config/syndicator.js?rev=632597&r1=632596&r2=632597&view=diff
==============================================================================
--- incubator/shindig/trunk/config/syndicator.js (original)
+++ incubator/shindig/trunk/config/syndicator.js Sat Mar 1 06:57:57 2008
@@ -79,4 +79,15 @@
// requests.
"useLegacyProtocol" : false,
},
+ // Skin defaults
+ "skins" : {
+ "properties" : {
+ "BG_COLOR": "",
+ "BG_IMAGE": "",
+ "BG_POSITION": "",
+ "BG_REPEAT": "",
+ "FONT_COLOR": "",
+ "ANCHOR_COLOR": "",
+ }
+ },
}}
Modified: incubator/shindig/trunk/features/skins/skins.js
URL: http://svn.apache.org/viewvc/incubator/shindig/trunk/features/skins/skins.js?rev=632597&r1=632596&r2=632597&view=diff
==============================================================================
--- incubator/shindig/trunk/features/skins/skins.js (original)
+++ incubator/shindig/trunk/features/skins/skins.js Sat Mar 1 06:57:57 2008
@@ -28,20 +28,48 @@
* currently shown skin.
* @name gadgets.skins
*/
-gadgets.skins = gadgets.skins || {};
+gadgets.skins = function() {
+ var skinProperties = {};
+
+ var requiredConfig = {
+ "properties": gadgets.config.ExistsValidator
+ };
+
+ gadgets.config.register("skins", requiredConfig, function(config) {
+ skinProperties = config["skins"].properties;
+ });
-/**
- * Fetches the display property mapped to the given key.
- *
- * @param {String} propertyKey The key to get data for;
- * keys are defined in <a href="gadgets.skins.Property.html"><code>
- * gadgets.skins.Property</code></a>
- * @return {String} The data
- *
- * @member gadgets.skins
- */
-gadgets.skins.getProperty = function(propertyKey) {};
+ return {
+ /**
+ * Override the default properties with a new set of properties.
+ *
+ * @param {Object} properties The mapping of property names to values
+ */
+ init : function(properties) {
+ skinProperties = properties;
+ },
+
+ /**
+ * Fetches the display property mapped to the given key.
+ *
+ * @param {String} propertyKey The key to get data for;
+ * keys are defined in <a href="gadgets.skins.Property.html"><code>
+ * gadgets.skins.Property</code></a>
+ * @return {String} The data
+ *
+ * @member gadgets.skins
+ */
+ getProperty : function(propertyKey) {
+
+ var property = skinProperties[propertyKey];
+ if (property) {
+ return property;
+ }
+ return "";
+ }
+ }
+}();
/**
* @static
* @class
@@ -71,8 +99,21 @@
FONT_COLOR : 'FONT_COLOR',
/**
+ * The positioning of the background image
+ * @member gadgets.skins.Property
+ */
+ BG_POSITION : 'BG_POSITION',
+
+ /**
+ * The repeat characteristics for the background image
+ * @member gadgets.skins.Property
+ */
+ BG_REPEAT : 'BG_REPEAT',
+
+ /**
* The color that anchor tags should use.
* @member gadgets.skins.Property
*/
ANCHOR_COLOR : 'ANCHOR_COLOR'
+
};
Re: svn commit: r632597 - in /incubator/shindig/trunk: config/syndicator.js features/skins/skins.js
Posted by Kevin Brown <et...@google.com>.
On Mon, Mar 3, 2008 at 12:09 AM, Paul Lindner <pl...@hi5.com> wrote:
> On 3/1/08 2:25 PM, "Kevin Brown" <et...@google.com> wrote:
> [....]
> >> + return {
> >> + /**
> >> + * Override the default properties with a new set of properties.
> >> + *
> >> + * @param {Object} properties The mapping of property names to
> values
> >> + */
> >> + init : function(properties) {
> >> + skinProperties = properties;
> >> + },
> >
> >
> > This seems redundant -- you already have it wired to the normal config
> > registration.
>
> Correct, the config registration handles the default skin properties
> for an entire site. Hi5 allows an individual to choose the skin for
> their profile page, which is passed into this init() method.
>
> Do you want to change the naming of this function? Or should I add
> more documentation to describe it's usage?
>
> Additionally we may want to allow overriding only a subset of
> properties. This is really a shindig implementation detail, we just
> need to decide how the container + syndicator + skins work together.
Assuming a theoretical skining format exists (say, something like igoogle
themes -- http://code.google.com/apis/themes/docs/dev_guide.html), it seems
that a good solution here would be to do the following:
- Pass the reference to the skin file into the iframe (a url would suffice,
but symbolic identifiers for skins available as local resources would be
better from a security standpoint, as Brian Eaton alluded to)
- Extract values from skin file and assign them to the skins API properties,
overriding the defaults.
- Assign this single block of values in the initialization.
I worry a little about having two initialization routines. Right now it's
difficult to wire up any sort of theming beyond per-site global theming
without maintaining some exhaustive proprietary patch, and I'd prefer a
single initialization call.
That said, what you've committed now is probably the best short-term
solution, and when we finally have well defined theme support (possibly in
the form of a spec standard, but I'll leave that discussion for another day)
we can go ahead and rip this out.
Will that require us to tweak the spec? If not I'll look at fixing
> This.
No, makeEnum takes care of this. At runtime all values resolve, but you
don't incur the extra bandwidth of all the declaration up front. Our code
doesn't have to look exactly like the spec apis, it just has to satisfy them
when a gadget is run under them. The spec is (necessarily) very verbose, but
our implementation is free to do whatever we want, as long as the API
conforms. For example, the following are equivalent from the spec's
standpoint, but very different from a performance and maintainability
standpoint:
gadgets.foo = function() {
function bar() {}
function baz()
return {
bar : bar,
baz : baz
};
}();
gadgets.foo = {
bar : function(){},
baz: function() {}
};
(function() {
gadgets.foo = {};
gadgets.foo.bar = function() {};
gadgets.foo.baz = function(){};
}());
All yield a gadgets.foo object with methods bar and baz, and thus satisfy
the "foo" API.
--
~Kevin
Re: svn commit: r632597 - in /incubator/shindig/trunk:
config/syndicator.js features/skins/skins.js
Posted by Paul Lindner <pl...@hi5.com>.
On 3/1/08 2:25 PM, "Kevin Brown" <et...@google.com> wrote:
[....]
>> + return {
>> + /**
>> + * Override the default properties with a new set of properties.
>> + *
>> + * @param {Object} properties The mapping of property names to values
>> + */
>> + init : function(properties) {
>> + skinProperties = properties;
>> + },
>
>
> This seems redundant -- you already have it wired to the normal config
> registration.
Correct, the config registration handles the default skin properties
for an entire site. Hi5 allows an individual to choose the skin for
their profile page, which is passed into this init() method.
Do you want to change the naming of this function? Or should I add
more documentation to describe it's usage?
Additionally we may want to allow overriding only a subset of
properties. This is really a shindig implementation detail, we just
need to decide how the container + syndicator + skins work together.
>> + getProperty : function(propertyKey) {
>> +
>> + var property = skinProperties[propertyKey];
>> + if (property) {
>> + return property;
>> + }
>> + return "";
>
>
> You can shorten this to return skinProperties[propertyKey] || "";
> (coincidentally, it looks like the skins API spec needs to be amended to
> be specific about what is supposed to be returned for invalid properties).
Okay, fix applied.
>> + /**
>> + * The repeat characteristics for the background image
>> + * @member gadgets.skins.Property
>> + */
>> + BG_REPEAT : 'BG_REPEAT',
>> +
>> + /**
>> * The color that anchor tags should use.
>> * @member gadgets.skins.Property
>> */
>> ANCHOR_COLOR : 'ANCHOR_COLOR'
>> +
>> };
>
> We should probalby use gadgets.util.makeEnum() for this.
Will that require us to tweak the spec? If not I'll look at fixing
This.
Re: svn commit: r632597 - in /incubator/shindig/trunk: config/syndicator.js features/skins/skins.js
Posted by Kevin Brown <et...@google.com>.
On Sat, Mar 1, 2008 at 6:57 AM, <li...@apache.org> wrote:
> Author: lindner
> Date: Sat Mar 1 06:57:57 2008
> New Revision: 632597
>
> URL: http://svn.apache.org/viewvc?rev=632597&view=rev
> Log:
> Implement skins feature as described in SHINDIG-98
>
> Modified:
> incubator/shindig/trunk/config/syndicator.js
> incubator/shindig/trunk/features/skins/skins.js
>
> Modified: incubator/shindig/trunk/config/syndicator.js
> URL:
> http://svn.apache.org/viewvc/incubator/shindig/trunk/config/syndicator.js?rev=632597&r1=632596&r2=632597&view=diff
>
> ==============================================================================
> --- incubator/shindig/trunk/config/syndicator.js (original)
> +++ incubator/shindig/trunk/config/syndicator.js Sat Mar 1 06:57:57 2008
> @@ -79,4 +79,15 @@
> // requests.
> "useLegacyProtocol" : false,
> },
> + // Skin defaults
> + "skins" : {
> + "properties" : {
> + "BG_COLOR": "",
> + "BG_IMAGE": "",
> + "BG_POSITION": "",
> + "BG_REPEAT": "",
> + "FONT_COLOR": "",
> + "ANCHOR_COLOR": "",
> + }
> + },
> }}
>
> Modified: incubator/shindig/trunk/features/skins/skins.js
> URL:
> http://svn.apache.org/viewvc/incubator/shindig/trunk/features/skins/skins.js?rev=632597&r1=632596&r2=632597&view=diff
>
> ==============================================================================
> --- incubator/shindig/trunk/features/skins/skins.js (original)
> +++ incubator/shindig/trunk/features/skins/skins.js Sat Mar 1 06:57:57
> 2008
> @@ -28,20 +28,48 @@
> * currently shown skin.
> * @name gadgets.skins
> */
> -gadgets.skins = gadgets.skins || {};
> +gadgets.skins = function() {
> + var skinProperties = {};
> +
> + var requiredConfig = {
> + "properties": gadgets.config.ExistsValidator
> + };
> +
> + gadgets.config.register("skins", requiredConfig, function(config) {
> + skinProperties = config["skins"].properties;
> + });
>
> -/**
> - * Fetches the display property mapped to the given key.
> - *
> - * @param {String} propertyKey The key to get data for;
> - * keys are defined in <a href="gadgets.skins.Property.html"><code>
> - * gadgets.skins.Property</code></a>
> - * @return {String} The data
> - *
> - * @member gadgets.skins
> - */
> -gadgets.skins.getProperty = function(propertyKey) {};
>
> + return {
> + /**
> + * Override the default properties with a new set of properties.
> + *
> + * @param {Object} properties The mapping of property names to values
> + */
> + init : function(properties) {
> + skinProperties = properties;
> + },
This seems redundant -- you already have it wired to the normal config
registration.
>
> +
> + /**
> + * Fetches the display property mapped to the given key.
> + *
> + * @param {String} propertyKey The key to get data for;
> + * keys are defined in <a href="gadgets.skins.Property.html
> "><code>
> + * gadgets.skins.Property</code></a>
> + * @return {String} The data
> + *
> + * @member gadgets.skins
> + */
> + getProperty : function(propertyKey) {
> +
> + var property = skinProperties[propertyKey];
> + if (property) {
> + return property;
> + }
> + return "";
You can shorten this to return skinProperties[propertyKey] || "";
(coincidentally, it looks like the skins API spec needs to be amended to
be specific about what is supposed to be returned for invalid properties).
>
> + }
> + }
> +}();
> /**
> * @static
> * @class
> @@ -71,8 +99,21 @@
> FONT_COLOR : 'FONT_COLOR',
>
> /**
> + * The positioning of the background image
> + * @member gadgets.skins.Property
> + */
> + BG_POSITION : 'BG_POSITION',
> +
> + /**
> + * The repeat characteristics for the background image
> + * @member gadgets.skins.Property
> + */
> + BG_REPEAT : 'BG_REPEAT',
> +
> + /**
> * The color that anchor tags should use.
> * @member gadgets.skins.Property
> */
> ANCHOR_COLOR : 'ANCHOR_COLOR'
> +
> };
We should probalby use gadgets.util.makeEnum() for this.
--
~Kevin