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

[DT] Fix 1052. Fires `contentUpdate ` after the DataTable has been updated. #1072

Merged
merged 4 commits into from Aug 31, 2013

Conversation

Projects
None yet
2 participants
@apipkin
Copy link
Contributor

apipkin commented Aug 11, 2013

Fixes issue #1052. There was no way to know when the table had completed updating when the ModelList was changed. This addresses this issue by firing contentUpdate after the table has completed updating.

  • Test
  • Code Fix
  • History
  • Sign Off or 3 days
@apipkin

This comment has been minimized.

Copy link
Contributor

apipkin commented Aug 11, 2013

@ericf

This comment has been minimized.

Copy link
Member

ericf commented Aug 12, 2013

changeUpdate doesn't make sense to me as the event name, I have no idea what it means when looking at it.

Does the internal model list's change event bubble to to the DT instance object? If this new event is intended to be the analog of that but for the UI being updated, then how about we call it contentUpdate, like Widget does?

@ericf

This comment has been minimized.

Copy link
Member

ericf commented Aug 12, 2013

Also since a DT is a Widget, you can fire this event, it already exists.

@apipkin

This comment has been minimized.

Copy link
Contributor

apipkin commented Aug 13, 2013

@ericf contentUpdate is perfect!! Thanks @ericf!

@@ -641,6 +642,7 @@ Y.namespace('DataTable').BodyView = Y.Base.create('tableBody', Y.View, [], {
// tbody
if (col.hasOwnProperty('nodeFormatter')) {
this.render();
this.fire(EV_CONTENT_UPDATE);

This comment has been minimized.

@ericf

ericf Aug 13, 2013

Member

Why would you fire this event inside a loop?

This comment has been minimized.

@apipkin

apipkin Aug 13, 2013

Contributor

This is only being fired when we are going to exit early. If there is a nodeFormatter present in any of the columns, we render the full table again, fire the contentUpdate event and return out of the method, not just the loop.

@ericf

This comment has been minimized.

Copy link
Member

ericf commented Aug 13, 2013

@apipkin why fire the event on the DataTable.BodyView instead of the DataTable itself?

@apipkin

This comment has been minimized.

Copy link
Contributor

apipkin commented Aug 13, 2013

@ericf we want to know when the table has updated after the content has been changed. Since the method _afterDataChange is bound and defined within the body.js file, I felt it was best to do the updating there.

Another option would be to fire contentUpdated from the body view, and or head and footer views. Then in base.js, where Y.Widget is extended, bind to *:contentUpdated and fire a single datatable:contentUpdated event.

Either way, an event needs to fire from body.js to alert the end user when the body has been updated, at least to my knowledge. I'm fine putting it elsewhere, but I'm lost on a better spot.

Merge branch 'dev-3.x' of https://github.com/yui/yui3 into dt-1052
Conflicts:
	src/datatable/HISTORY.md

@apipkin apipkin merged commit 0939142 into yui:dev-3.x Aug 31, 2013

1 check failed

default The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment