Add tooltip component #2843
Add tooltip component #2843
Conversation
|
No need for both the issue and the PR on the milestone |
|
I'd love to hear opinions on the deprecation warning added in 356df4d. I don't know how I feel about the way this is implemented, but it was the best I could think of. Maybe my brain at a time other than 3am might be able to do a better job! It only serves to inform extension devs that their current tooltip method is deprecated and that they should migrate to the new version. |
|
I've forgotten to implement I'll do that tomorrow. |
There was a problem hiding this comment.
The more I look at this, the less I like that the tooltip is created as an additional HTML node. Adding a tooltip is a modification to an existing element, not the creation of a new element.
Would pseudoelement conflicts arise when a JS tooltip is used? Conceptually a CSS-only tooltip is cool, but the more I see workarounds needed to implement it, the less I'm convinced that CSS is ready.
Will be used to provide backwards compatibility when we switch to CSS tooltips. All other methods of creating tooltips are deprecated and this component-based method should be used instead.
davwheat
force-pushed
the
dw/add-tooltip-component
branch
from
ce8645d to
677968b
Compare
Instead of using a container to house the tooltip, we'll now modify the first direct child of the Tooltip component. The Tooltip component will ensure that: - children are passed to it - only one child is present - that child is an actual HTML Element and not a text node, or similar - that child is currently present in the DOM Only after all of the above are satisfied, will the tooltip be created on that element. We store a reference to the DOM node that the tooltip should be created on, then use this to perform tooltip actions via jQuery. If this element gets changes (e.g. the tooltip content is updated to another element) then the tooltip will be recreated. If any of the first 3 requirements are not satisfied, an error will be thrown to alert the developer to their misuse of this component. To make this work, we do need to overwrite the title attribute of the element with the tooltip, but this is the only solution other than specifying `title` as an option when making the tooltip, but this is not accessible by screenreaders unless they simulate a hover on the element.
|
Ok, I've changed the component to apply the tooltip directly to the first child of the component. Along comes a lot of error detection measures to ensure devs find out if they're using this component incorrectly. For more info, see the message on commit 22ebd0b |
|
This has made the bundle a lot bigger, but a chunk of this can be shaved off if you don't think that manually showing/hiding tooltips is important. I personally don't at all, as tooltips shouldn't be used for that. |
| private updateVisibility() { | ||
| if (this.childDomNode === null) return; | ||
|
|
||
| if (this.attrs.tooltipVisible === true) { |
There was a problem hiding this comment.
These very type-specific checks are confusing to me: isn't tooltipVisible required to be boolean?
There was a problem hiding this comment.
It's bool or undefined. We need the strong checks to ensure we don't attempt to hide or show the tooltip when it's undefined.
We don't need the true one, but I thought matching them looked nicer and less like a mistake.
|
Other than these, code works locally. |
Prevents breaking changes as a result of #2697. Progresses flarum/issue-archive#179.
Changes proposed in this pull request:
This is still WIP. Created as a draft now to allow for any comments to be made while the component is still being made.
This adds a new Tooltip component to core. This should be used by extensions instead of
$.tooltipto aid in the shift to other tooltip libraries (hopefully CSS tooltips!) in the future.I've tried to include the main tooltip options that are used by extensions, such as
delayandhtml, but these won't be easy or possible to keep if we shift to CSS tooltips, so they've been marked as deprecated and only exist to allow extensions to more easily shift to this component.Reviewers should focus on:
Will the deprecation of
htmlanddelaycause an issue with #2697 as they aren't available in that? Do we consider removing deprecated features within a stable release ok, bearing in mind there is a fairly bold warning in the docblock, or should we just not implement them as part of the component in the first place.I think implementing them as deprecated features will help ext devs migrate to this component faster and more easily now, bearing in mind the large number of other changes they need to perform.
Should we also add a deprecation warning to the tooltip helper when it's called from outside the
Tooltipcomponent to make this more obvious to devs?Screenshot
Confirmed
composer test).Required changes: