Add customClass to bar hover hint

[jlorenzen]

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.

[usmonster]

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?

[jlorenzen]

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.

[usmonster]

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!

[jlorenzen]

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 notifications@github.com 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//pull/98#issuecomment-25850561
.

[usmonster]

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!

[jlorenzen]

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 notifications@github.com 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//pull/98#issuecomment-25897363
.

[usmonster]

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. :]

[jlorenzen]

Ah very good point on externally defined styles. I hadn't thought of that.

On Tue, Oct 8, 2013 at 10:40 AM, usmonster notifications@github.com 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//pull/98#issuecomment-25901376
.