Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved support for single-box Widgets #1125

Merged
merged 3 commits into from
Aug 28, 2013
Merged

Conversation

derek
Copy link
Contributor

@derek derek commented Aug 23, 2013

Rendering a widget that has CONTENT_TEMPLATE = null and a srcNode ATTR will result in destruction of the original node as the bounding box replaces it. This is unfortunate because it prevents a use-case where you want the content box (srcNode) and bounding box to point to the same pre-existing node.

This fixes that issue by introducing a boundingBox valueFn to check for a srcNode if CONTENT_TEMPLATE is null. If so, it defaults boundingBox to srcNode. If not, it reverts to the original behavior (returning null, which creates a bounding box from BOUNDING_TEMPLATE).

A full-library run of yogi lint checks out fine, as well as select Widgets via Yeti:

Widget

✓ Agent completed: Chrome (28.0.1500.95) / Mac OS
✓ Agent completed: Firefox (23.0) / Mac OS
✓ Agent completed: Internet Explorer (10.0) / Windows 8
✓ Agent completed: Internet Explorer (8.0) / Windows XP
✓ Agent completed: Internet Explorer (6.0) / Windows XP
✓ 753 tests passed! (21 seconds)

Autocomplete

✓ Agent completed: Chrome (28.0.1500.95) / Mac OS
✓ Agent completed: Internet Explorer (10.0) / Windows 8
✓ Agent completed: Internet Explorer (8.0) / Windows XP
✓ Agent completed: Firefox (23.0) / Mac OS
✓ Agent completed: Internet Explorer (6.0) / Windows XP
✓ 452 tests passed! (2 minutes, 12 seconds)

Datatable

✓ Agent completed: Chrome (28.0.1500.95) / Mac OS
✓ Agent completed: Firefox (23.0) / Mac OS
✓ Agent completed: Internet Explorer (10.0) / Windows 8
✓ Agent completed: Internet Explorer (8.0) / Windows XP
✓ Agent completed: Internet Explorer (6.0) / Windows XP
✓ 1498 tests passed! (3 minutes, 26 seconds)

Original Ticket: http://yuilibrary.com/projects/yui3/ticket/2530076

A widget with single box format (CONTENT_TEMPLATE = null) will destroy the contents of srcNode on render.

new MyWidget({
   srcNode: '#demo'   <-- will be destroyed on render
});

Expected behavior would be to use the provided srcNode as the boundingBox and contentBox.

@derek derek mentioned this pull request Aug 23, 2013
5 tasks
@@ -181,7 +181,7 @@ ATTRS[RENDERED] = {
* @writeOnce
*/
ATTRS[BOUNDING_BOX] = {
value:null,
valueFn:"_defaultBB",
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test which subclasses Widget and sets a custom value for the boundingBox attribute to make sure that the subclass' value wins over Widget's valueFn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with a few additional tests. Let me know if they don't address the test case you were looking for.

@derek derek merged commit 8cbf367 into yui:dev-master Aug 28, 2013
@drjayvee
Copy link

drjayvee commented Sep 9, 2013

@derek, I noticed that ButtonGroup always wraps its srcNode in a yui3-buttongroup container. Is this going to change? I don't see the need for two containers (yui3-buttongroup and yui3-buttongroup-content), and I would have to adjust my CSS selectors if it does change.

@derek
Copy link
Contributor Author

derek commented Sep 9, 2013

@drjayvee As of now, the wrapper class for the ButtonGroup widget will remain as yui3-buttongroup, and I don't expect any changes there. I do agree that ButtonGroup widgets should be single-box widgets as well, which would simply mean updating group.js to include a null CONTENT_TEMPLATE. Though, as you alluded to, changing this could be considered a backwards incompatibility, so I'll give it a bit more thought first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants