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

[Widgets] Add more attributes to some widgets#123

Closed
maxyu wants to merge 9 commits into
intel:masterfrom
maxyu:attribute-3
Closed

[Widgets] Add more attributes to some widgets#123
maxyu wants to merge 9 commits into
intel:masterfrom
maxyu:attribute-3

Conversation

@maxyu

@maxyu maxyu commented Jul 6, 2012

Copy link
Copy Markdown
Contributor

No description provided.

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.

Can we extend it from BCommonProperties.theme?

@maxyu

maxyu commented Jul 6, 2012

Copy link
Copy Markdown
Contributor Author

Updated. Please check.

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.

Default value should be false rather than "false"

@zhizhangchen zhizhangchen mentioned this pull request Jul 9, 2012
@grgustaf

Copy link
Copy Markdown
Contributor

Since the follow-on patches are mostly fixes to the initial patch, they should all be merged into one patch. That would make it easier to review as well.

Multiple patches in a pull request should be split up based on different functionality that they add, but fixups should be merged in. Here the changes are all loosely related so it's okay for it to be one big patch, but it would also be easier to swallow if some parts were in their own patches (e.g. "new button properties" or "add nativecontrol property to several widgets"). (If those were independent I'd have already merged them.)

Note: "git add -p" is an awesome feature for splitting up a patch into multiple ones when you've made too many changes together, FWIW. But not required here.

@maxyu

maxyu commented Jul 11, 2012

Copy link
Copy Markdown
Contributor Author

replaced by PR#139

@maxyu maxyu closed this Jul 11, 2012
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