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

Expand customizable cells to all views #2124

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Azaret
Copy link
Contributor

@Azaret Azaret commented Feb 14, 2017

Q A
Bug fix? no
New feature? rather enhancement
BC breaks? no
Related tickets fixes #2120, partially #2043
License MIT

Copy link
Contributor Author

@Azaret Azaret left a comment

Choose a reason for hiding this comment

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

WIP

@Azaret Azaret added this to the 1.7.0 milestone Feb 14, 2017
@Azaret Azaret changed the title WIP: Expand customizable cells to all views Expand customizable cells to all views Feb 14, 2017
@Azaret
Copy link
Contributor Author

Azaret commented Feb 14, 2017

@vsn4ik I took the liberty to enhance your idea.
I directly attach the event listener to the correct DOM node and use jQuery data() so the DOM is kept clear.

@Azaret
Copy link
Contributor Author

Azaret commented Feb 14, 2017

Attaching the event directly to the node was in my mind for the v2.
But the click function is already less a mess than 2 minors versions ago, so it's safe to make the change now.

@Azaret
Copy link
Contributor Author

Azaret commented Feb 14, 2017

@acrobat Fill free to play around to be sure I didn't break everything.

Some snippet for your amusement:

function callback() {
    var smileys = ['😁', '😋', '😡', '😰', '🙅', '🙊', '🙏', '✌', '🚀', '🚨', '🚧', '🚽', '🉐', '⌚', '☝', '⚽', '⚾', '⛄', '⛅'];
    return {content: '<div>' + smileys[Math.floor(Math.random() * smileys.length)] + '</div>'};
}

$('input').datepicker({
    beforeShowDay:     callback,
    beforeShowMonth:   callback,
    beforeShowYear:    callback,
    beforeShowDecade:  callback,
    beforeShowCentury: callback
});

@acrobat
Copy link
Member

acrobat commented Feb 15, 2017

@Azaret Thanks for the work! I've tested the code and it looks like it works for all views (cool example btw :D)

@vsn4ik @hebbet Do you guys have time for a review?

@acrobat acrobat requested review from hebbet and vsn4ik February 15, 2017 07:21
@acrobat
Copy link
Member

acrobat commented Feb 15, 2017

@Azaret Do we need extra tests for this behavior (the changes to the click etc) or is it covered by the current tests?

node.data('date', {day: 1, month: i, year: this.viewDate.getUTCFullYear()});
node.attr('class', 'month' + focused);
node.html(dates[this.o.language].monthsShort[i]);
node.on('click', $.proxy(this.cellClick, this));
Copy link
Member

Choose a reason for hiding this comment

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

No, it is bad practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why ?

Copy link
Member

Choose a reason for hiding this comment

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

If you follow your rule will turn 90 listeners: (4 * 12) + 42 (days) = 90. Is not critical, it is possible to leave.

Copy link
Member

Choose a reason for hiding this comment

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

Is this comment fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I though about it the other day and it hit me what @vsn4ik was talking about.
The issue is that by design it is not easy to catch the event when the DOM is customized.
Comprehensive drawing:
image

Before it was easy, you can bind on the container, and get the source of the event with currentTarget.
Now currentTarget will return the custom DOM nodes instead of the td cell.

The easiest and safest way is then to bind the event to the cell, so target will equals the cell, always.
If we keep the event bound to the container, it mean we'll have to lookup for the cell between the target and the currentTarget.

So the question is, which is the best performance wise between binding 90 events or recursively find a DOM element.

Copy link
Member

Choose a reason for hiding this comment

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

Ok that explains a lot! So we should go for the 90 events for now? What do you think @vsn4ik?

Copy link
Member

Choose a reason for hiding this comment

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

Ping @vsn4ik

@acrobat
Copy link
Member

acrobat commented Feb 20, 2017

@Azaret @vsn4ik I'm going to release RC2 in the next few days. If this is complete then it will be in RC otherwise we will do another RC when this is merged!

@Azaret
Copy link
Contributor Author

Azaret commented Feb 23, 2017

Well it looks clear to me, I'm waiting @vsn4ik input on his remarks.

@acrobat
Copy link
Member

acrobat commented Feb 23, 2017

Hmm he had posted a comment, but it looks like it was deleted. So indeed wait for @vsn4ik for his feedback

@brandon-burciaga
Copy link

brandon-burciaga commented Mar 16, 2017

When is this expected to come out? According to the docs, returning an object with a content key in the beforeShowDay method should allow me to change the content, however it does not work. I can disable a day fine with a boolean, just can't set content. @Azaret @acrobat @vsn4ik

edit* I realized this isn't enabled in the version I am using (1.6.4) and I could change the docs based on version :) thanks to everyone contributing to this

@acrobat
Copy link
Member

acrobat commented Mar 19, 2017

@Azaret sorry to ping you again! But any more work needed here? If I understand your comment above correctly "90 events" choice we have to make is good enough for now?

If we can agree on some solution for this would be nice, this way I can put out a RC2 ( and hopefully quite quick a stable release)

/cc @vsn4ik

@acrobat acrobat modified the milestones: 1.7.0, 1.8.0 Jun 17, 2017
@vsn4ik vsn4ik removed this from the 1.8.0 milestone Apr 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand customizable cells #2043 to all views
4 participants