taitems
Add customClass to bar hover hint
Updated the bar mouseover hint to also include the customClass. This will give developers more control over the color of the hover. Currently is defaults to a yellow. Now developers can customize the color of the hover based on the gantt Color passed in to the values.
Thanks for using the plugin, and even more for submitting a pull request!
And while I wholeheartedly agree that the popover should be customizable, I'm not sure what generic advantage the custom class would have to a simple CSS override of the fn-gantt-hint class?
If you just override fn-gantt-hint that will apply to all hovers. For example, by default all hovers are yellow no matter what the customClass value is. So my progress bar is ganttRed, but the hover is yellow. I want to be able to customize the hover to be red as well, if the progress bar is ganttRed.
Ah, yes, good idea. Nice straightforward implementation, though it may have unexpected side effects where users of the plugin already have very specific rules for their custom classes. Merging this would require these users to take action, which can be disruptive.
I would instead propose changing how the hint itself is appended. It might be best to attach the hint to the bar itself, instead of to the body as it is done now. (Related note to self: It would also be a better practice to keep a reference to the hint and call detach() instead of continually creating and remove()ing the element.)
This would give us a clean selector (e.g. .fn-gantt .bar.ganttRed .fn-gantt-hint) that allows us to style the hint how we want without having to change or override existing rules for the bar's custom class. Would you be willing to try that out and maybe make a new pull request? Thanks again!
Yeah I can give that a shot. I'm not sure I understand your first comment though. I don't think it would hurt any existing users unless they have modified the source themselves, at which point I don't think you can avoid preventing issues if they modify the source and upgrade to a newer version. Currently, the only selector that gives them control for the hover is .fn-gantt-hint. I don't think adding another class to the hover div tag would break anything in that situation. For example, let's say I wanted to make the text bold in the hover, I would add this selector to my css:
.fn-gantt-hint{font-weight: bold;}
And if that developer got a new version where we add the customClass to the hover div tag, that developers style still works.
Again, I may not understand everything completely and might/probably am missing something.
If you still want me to, I can try to make those other changes you recommended. Let me know.
On a side note, I also thought it was strange how it was getting added to the body. I was looking for it in chrome and could never find it. I finally had to comment the remove call so I could inspect it.
Let me know if my changes are still good enough or if you think it's worth it to make the changes you suggest.
On Mon, Oct 7, 2013 at 5:22 PM, usmonster [email protected] wrote:
Ah, yes, good idea. Nice straightforward implementation, though it may have unexpected side effects where users of the plugin already have very specific rules for their custom classes. Merging this would require these users to take action, which can be disruptive.
I would instead propose changing how the hint itself is appended. It might be best to attach the hint to the bar itself, instead of to the body as it is done now. (Related note to self: Calling detach() instead of remove()on mouseout would also be a better practice.)
This would give us a clean selector (e.g. .fn-gantt .bar.ganttRed .fn-gantt-hint) that allows us to style the hint how we want without having to change or override existing rules for the bar's custom class. Would you be willing to try that out and maybe make a new pull request? Thanks again!
— Reply to this email directly or view it on GitHubhttps://github.com/taitems/jQuery.Gantt/pull/98#issuecomment-25850561 .
Thanks for the response!
Regarding my comment on possible side effects, I was referring to the fact that your patch as-is adds to the hint div the same customClass that current users of the plugin expect only to appear on the bars. Thus the styles are newly applied to the hint element, which is a change in the default behavior of the plugin. This might be unwanted by many users and may require that they rework their existing CSS after an upgrade if they wanted to go back to the old/default appearance.
The modifications I proposed should avoid this side effect since the class is not directly applied to the hint, but I haven't tested it out. Please let me know if you decide to implement the suggestions in a new pull request!
Oh right. I see now. I think the only way current users would see that issue with the current patch as-is would be if they defined a selector override of just .ganttRed instead of say .bar.ganttRed.
Either way, I'm guessing you'd prefer I implement a new pull request with your suggestions? I'll try to look into that tonight.
On Tue, Oct 8, 2013 at 9:58 AM, usmonster [email protected] wrote:
Thanks for the response!
Regarding my comment on possible side effects, I was referring to the fact that your patch as-is adds to the hint div the same customClass that current users of the plugin expect only to appear on the bars. Thus the styles are newly applied to the hint element, which is a change in the default behavior of the plugin. This might be unwanted by many users and may require that they rework their existing CSS after an upgrade if they wanted to go back to the old/default appearance.
The modifications I proposed should avoid this side effect since the class is not directly applied to the hint, but I haven't tested it out. Please let me know if you decide to implement the suggestions in a new pull request!
— Reply to this email directly or view it on GitHubhttps://github.com/taitems/jQuery.Gantt/pull/98#issuecomment-25897363 .
The other common use case to watch for would be when the customClass isn't one that's given in the plugin's own stylesheet, but one that is used in externally-defined styles. In this case, we cannot predict the effect of adding the class to the hint div.
In any case, a new PR would be great, though no worries if you can't get around to it. It's all open development, and the change may eventually be implemented anyway. :]
Ah very good point on externally defined styles. I hadn't thought of that.
On Tue, Oct 8, 2013 at 10:40 AM, usmonster [email protected] wrote:
The other common use case to watch for would be when the customClassisn't one that's given in the plugin's own stylesheet, but one that is used in externally-defined styles. In this case, we cannot predict the effect of adding the class to the hint div.
In any case, a new PR would be great, though no worries if you can't get around to it. It's all open development, and the change may eventually be implemented anyway. :]
— Reply to this email directly or view it on GitHubhttps://github.com/taitems/jQuery.Gantt/pull/98#issuecomment-25901376 .