Skip to content
This repository was archived by the owner on Mar 14, 2020. It is now read-only.

[Widget] Support background images#84

Merged
sbryan merged 2 commits into
intel:masterfrom
wuyongfeng:backgroundimg-widget
Jun 26, 2012
Merged

[Widget] Support background images#84
sbryan merged 2 commits into
intel:masterfrom
wuyongfeng:backgroundimg-widget

Conversation

@wuyongfeng

Copy link
Copy Markdown

Page, the Header, Footer have been achieved, other div inherit the "Background"

Comment thread src/js/widgets.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use htmlAttribute mapping to achieve this functionality, although htmlAttribute may have to be extended.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This difference in the IMAGE WIDGET for can not be used

@wuyongfeng

Copy link
Copy Markdown
Author

This difference in the IMAGE WIDGET for can not be used

yongfeng wu added 2 commits June 25, 2012 14:30
Page, Header and Footer have been supported, other widget can
inherit the "Background" widget to get background support
@zhizhangchen zhizhangchen mentioned this pull request Jun 25, 2012
Comment thread src/js/widgets.js

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in-line styles are "BAD"... absolutely not something we want to be promoting or encouraging developers to use or depend on.

While I'll accept this commit for initial implimentation, we REALLY need to move towards creating and maintaining a custom per-application CSS mapping that we generate during serialize steps, like for layout, code and live views, as well as on export. Then, these additional style attributes become available to be changed via CSS editing rather than adding more and more properties directly to the widgets.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in the long run, we should do that.

@sbryan

sbryan commented Jun 26, 2012

Copy link
Copy Markdown
Contributor

Why only available on Page, Header and Footer widgets? It would not be unreasonable to allow image backgrounds on ALL widgets by default, and only exclude it for the exceptions (when we find them). Then we don't need to introduce a new widget class, just enhance Base, right?

@sbryan sbryan merged commit ac9d202 into intel:master Jun 26, 2012
@zhizhangchen

Copy link
Copy Markdown
Contributor

The current inheritence mechanism doesn't allow the excluding of properties in base widget from extended widget. From Object oriented point of view, we shouldn't do that actually.

@sbryan

sbryan commented Jun 26, 2012

Copy link
Copy Markdown
Contributor

On Mon, Jun 25, 2012 at 8:45 PM, Chen, Zhang Z
reply@reply.github.com
wrote:

The current inheritence mechanism doesn't allow the excluding of properties in base widget from extended widget. From Object oriented point of view, we shouldn't do that actually.

I don't think this is true from OO perspective. OO simply means
sub-class is descended from parent class, containing all the
properties and methods, with some additions, but ALSO allows for
default values to be overridden or methods to be replaced with it's
own versions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants