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

Allow customizing day cell value #2043

Merged
merged 7 commits into from
Feb 12, 2017

Conversation

programcsharp
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
BC breaks? no
License MIT

Copy link
Contributor

@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.

Hi, Thank you for your work. Please don't forget to add some unit tests.

@@ -1196,6 +1200,10 @@
}

if (!target.hasClass('disabled')){
// Allow clicking on elements inside a day
if (target.parent().hasClass('day'))
target = target.parent();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you allow html, you code won't work if I send <div><div></div></div>, you should change e.target by e.currentTarget, but then you should do a full test to be sure it does not break everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that e.currentTarget would work because bubbling/propagation are stopped at the top of the click handler.

Instead, I use .parents(), does that work?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should I guess

@@ -1041,7 +1045,7 @@
clsName = $.unique(clsName);
}

html.push('<td class="'+clsName.join(' ')+'"' + (tooltip ? ' title="'+tooltip+'"' : '') + (this.o.dateCells ? ' data-date="'+(prevMonth.getTime().toString())+'"' : '') + '>'+prevMonth.getUTCDate() + '</td>');
html.push('<td class="'+clsName.join(' ')+'"' + (tooltip ? ' title="'+tooltip+'"' : '') + (this.o.dateCells ? ' data-date="'+(prevMonth.getTime().toString())+'"' : '') + '>' + displayDate + '</td>');
Copy link
Contributor

Choose a reason for hiding this comment

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

You are actually making the other views sad and envious to the monthView :)
May you add this on the other views please ?

@@ -1017,6 +1017,8 @@
clsName = this.getClassNames(prevMonth);
clsName.push('day');

var displayDate = prevMonth.getUTCDate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since other tickets like #2037 would like such PR, I think a more abstract name for this option should be use, like content. Do you guys agree @acrobat @vsn4ik @hebbet ?

Copy link
Member

Choose a reason for hiding this comment

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

👍

Add test to check click on custom content
@programcsharp
Copy link
Contributor Author

  • Changed option name to content
  • Changed click handler to use .parents() to find .day
  • Added generic test for content
  • Added test for clicking inside nested content

Anything else need done here?

@@ -1196,6 +1200,12 @@
}

if (!target.hasClass('disabled')){
// Allow clicking on elements inside a day
var day = target.parents('.day');
Copy link
Member

Choose a reason for hiding this comment

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

Day is already defined on line 1183, see travis build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@Azaret
Copy link
Contributor

Azaret commented Oct 28, 2016

Yeah, I would like to see the same feature for the others views (month, year, decade, ...) lookup for beforeShowMonth for month and callback inside _fill_yearsView() for others.
Ty

@programcsharp
Copy link
Contributor Author

OK, I can do that for consistency and all that goodness. I'll fix the var name too.

Do you have a better suggestion for how to handle deeply nested clicks? I can duplicate the same thing I did for day -- target.parents(".day, .month, .year"), but that just seems non-optimal.

@Azaret
Copy link
Contributor

Azaret commented Oct 28, 2016

Do it for the fame ;)

target.closest() should be better performance wise I think, since it will stop at the first parent match.
It should be enough for now, we might break the click function in the 2.x, since binding events directly to the actual td will be easier to manage than catching any click anywhere on the picker (which by the way is a broken way of doing things because it does trigger the show event at any clicks).

@brandon-burciaga
Copy link

Being able to customize content within each day would be really awesome. I am trying to display counts of something in each day but haven't been able to figure out how.

@acrobat
Copy link
Member

acrobat commented Jan 9, 2017

@Azaret Can you review the pull request again? Thanks!

@acrobat
Copy link
Member

acrobat commented Feb 2, 2017

@programcsharp I would like to get this PR finished and have it shipped with 1.7.0! 🚀 Do you need to do any work here? Please rebase the branch against the current mast and can you check the failing travis build? Thanks!

@Azaret Time for a last review? Thanks!

@programcsharp
Copy link
Contributor Author

OK, lemme see what I can do here.

@programcsharp
Copy link
Contributor Author

Fixed the dupe variable name issue @Azaret mentioned. Should be ready to go now.

I don't have time to add the month/year/decade/century implementations.

@acrobat
Copy link
Member

acrobat commented Feb 6, 2017

Thanks @programcsharp! I will look into this to see what I can do to have this feature shipped with 1.7.0

@Azaret
Copy link
Contributor

Azaret commented Feb 12, 2017

I guess we can push it in RC2 @acrobat

@acrobat
Copy link
Member

acrobat commented Feb 12, 2017

@Azaret Yes I will merge this today and tag a new RC release. But we need to try to implement this feature for all beforeXxx methods, I hope to find some time for this!

@acrobat acrobat merged commit 3b1e472 into uxsolutions:master Feb 12, 2017
@acrobat
Copy link
Member

acrobat commented Feb 12, 2017

Thanks @programcsharp! The missing parts for beforeShowMonth (yeay, decade, etc) will be done in #2120

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.

4 participants