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

Don't let data-{classPrefix} attribute override content option #134

Merged
merged 3 commits into from
Jan 4, 2016

Conversation

TrevorBurnham
Copy link
Contributor

Noticed this while playing around with a solution to HubSpot/tooltip#50. Currently, if the Drop target has a data-{classPrefix} attribute, that attribute is used as the Drop content regardless of whether options.content is set.

As far as I can tell, this feature is undocumented, and it's quite confusing. For instance, Tooltip sets options.content based on data-tooltip on its own, even though the content option is actually ignored by Drop (since Drop's classPrefix is set to tooltip, causing it to take the value of data-tooltip directly as its content).

@huoxito This is your code (#107), so what do you think? It seems to me that JavaScript options should take precedence over DOM attributes, but maybe I'm missing some use case. Either way, there should be documentation for this feature. /cc @geekjuice

@TrevorBurnham TrevorBurnham mentioned this pull request Dec 31, 2015
@geekjuice
Copy link
Contributor

@TrevorBurnham I think the DOM > JS was an arbitrary decision. I would be fine with checking to ensure that there are are JS options passed in before overriding the content attribute. As far as documentation, it could use some overhaul all across our projects, but I agree we should at least mention the ability to set options via DOM attributes.

@huoxito
Copy link
Contributor

huoxito commented Jan 4, 2016

@TrevorBurnham yeah the PR missed docs sorry about that guys, I'm not sure I follow the tooltip options.content issue but I'll take another look.

About the DOM > JS by looking at the other PR I think the motivation was to keep same behavior as css inline styles but I'm fine either way, at least I don't see how it would bother us.

@TrevorBurnham
Copy link
Contributor Author

I'm not sure I follow the tooltip options.content issue but I'll take another look.

  1. Tooltip creates a Drop context in which classPrefix is tooltip. It has to do this in order to be able to style Drops with classes like tooltip-open.
  2. The documented API for Tooltip asks users to use the data-tooltip attr to specify the textual content of the tooltip attached to an element.
  3. Because data-tooltip exists on those elements and classPrefix is tooltip, Drop uses that attribute as the content and ignores the content option that Tooltip passes in. That's a problem because I was thinking of making Tooltip to create an element, add some attributes to it, and pass that in as the content option.

Here's an alternative solution: We could make Drop accept an attributes option and add those to the content element in setupElements. Actually that might make more sense... I'll take a crack at it.

@TrevorBurnham
Copy link
Contributor Author

I'm going back and forth on this. Adding an attributes seems redundant when the content option can be a DOM element. Given that there's currently no documentation for setting options via DOM attributes, I'm inclined to merge this PR as-is.

TrevorBurnham added a commit that referenced this pull request Jan 4, 2016
Don't let data-{classPrefix} attribute override content option
@TrevorBurnham TrevorBurnham merged commit 6d3037f into master Jan 4, 2016
@TrevorBurnham TrevorBurnham deleted the dont-override-content-option branch January 4, 2016 17:37
TrevorBurnham pushed a commit that referenced this pull request Jan 4, 2016
TrevorBurnham pushed a commit to HubSpot/tooltip that referenced this pull request Jan 4, 2016
This adds the `role=tooltip` attr to the tooltip content, and the
`aria-describedby` attr on the tooltip target.

Requires Drop 1.4.2, due to this change:
HubSpot/drop#134
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants