feat(module:overlay): OverlayTrigger not bound to a div #937

anddrzejb · 2020-12-29T13:06:00Z

🤔 This is a ...

🔗 Related issue link

Eventually if implemented, fixes #898 and #383

💡 Background and solution

For the div-free solution, the parent element (for example Tooltip) needs to have a reference to its child. Here it is solved using ForwardRef and template approach. This solution if applied to currently existing OverlayTrigger, would unfortunately represent a breaking change. So for purpose of discussion and additional work, I created new versions of OverlayTrigger, Overlay and Tooltip with suffix Unbound:

OverlayTriggerUnbound
OverlayUnbound
TooltipUnbound

Unbound keyword should indicate that this is an overlay that is not bound to a div (or actually to anything). In the tooltip Basic.razor demo file I also created a tooltip that uses the new approach.

In below gif notice that 1st tooltip has a wrapper (uses the regularTooltip) and 2nd tooltip does not have any wrapper but directly shows the tooltip's child element <span> (uses newly created TooltipUnbound).

Also - this example is not yet fully done for trigger. Because the events can no longer be attached to div, they are attached to child element using the reference (ForwardRef). This PR in current state does not handle @oncontextmenu event. But it should handle all the other events that are defined on the regular Tooltip. I guess the @oncontextmenu event can be implemented in the same manner as the other events.

📝 Changelog

Language	Changelog
🇺🇸 English
🇨🇳 Chinese

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

Doc is updated/provided or not needed
Demo is updated/provided or not needed
Changelog is provided or not needed

codecov-io · 2020-12-29T13:22:30Z

Codecov Report

Merging #937 (2ed7f27) into master (ce117b5) will increase coverage by 0.01%.
The diff coverage is 4.05%.

@@            Coverage Diff            @@
##           master    #937      +/-   ##
=========================================
+ Coverage    5.69%   5.70%   +0.01%     
=========================================
  Files         412     422      +10     
  Lines       22030   22271     +241     
=========================================
+ Hits         1254    1270      +16     
- Misses      20776   21001     +225

Impacted Files	Coverage Δ
components/affix/Affix.razor	`0.00% <0.00%> (ø)`
components/affix/Affix.razor.cs	`0.00% <0.00%> (ø)`
components/anchor/Anchor.razor	`0.00% <0.00%> (ø)`
components/anchor/Anchor.razor.cs	`0.00% <0.00%> (ø)`
components/anchor/AnchorLink.razor	`0.00% <0.00%> (ø)`
components/anchor/AnchorLink.razor.cs	`0.00% <0.00%> (ø)`
components/auto-complete/AutoComplete.razor	`0.00% <0.00%> (ø)`
components/auto-complete/AutoComplete.razor.cs	`0.00% <ø> (ø)`
components/auto-complete/AutoCompleteInput.cs	`0.00% <0.00%> (ø)`
components/auto-complete/AutoCompleteSearch.cs	`0.00% <0.00%> (ø)`
... and 117 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce117b5...2ed7f27. Read the comment docs.

ElderJames · 2020-12-29T14:14:06Z

Thank you very much @anddrzejb , it help a lot.

But if you implement a new component with a lot of repetitive code, it would increase maintenance costs and confuse users.
Could you implement this feature by inheriting from the original component?

We might be more comfortable with using a new template parameter:

<Tooltip Title="@("prompt text")">
    <Unbound>
        <span  @ref="@context.Current">Tooltip (div-unbound) will show on mouse enter.</span>
    <Unbound>
</Tooltip>

However, I have to say that this is a great feature and I really hope to merge it into the project.

anddrzejb · 2020-12-29T14:29:20Z

I added this as a draft PR exactly for that purpose. I was hoping to get input on the idea and use others experience. Also, I did not want to spent a ton of time just to find out the solution is not acceptable. That is why I just did a code repeat - to get the discussion started as well as have a working example.

To address your point - at first was trying to inherit from original component, but for the solution to work, ChildComponent has to be overridden with ChildComponent<ForwardRef>. And this cannot be done (different types). And blazor does not allow to have 2 properties with name ChildComponent. That is why I was forced to use current solution. Of course, I could extract as much as possible of the common code to avoid code duplication (to base classes I guess). Here it is just a proof-of-concept. I will give another shot to your proposal but I am also open to suggestion on how to solve the issues I foresee...

anddrzejb · 2020-12-29T21:47:00Z

@ElderJames following your suggestion, I did some research on multiple child elements and managed to revert to single component use. This seems to work. However there is still one (minor?) problem - I did not manage to replicate @oncontextmenu:preventDefault for unbounded Tooltip. It seems to me that a change to interop.ts and DomEventService would be required (to add additional optional argument bool preventDefault = false).

Edit: apparently I messed up merging with upstream. All significant changes are in commit 8c39956. The rest is merging with upstream and syncing with branch...

ElderJames · 2020-12-30T09:50:35Z

Yes, a little bit of necessary JS is acceptable.

…design-blazor into overlayTriggerDiv

anddrzejb · 2020-12-30T10:23:06Z

@ElderJames now full functionality of Tooltip has been replicated in the <Unbound> version. Please have a look at the JS changes as this is not my strong suite (and connected changes in DomEventService).
I will stop working on this until you tell me to either fix something or that everything is fine.
If you accept this change, I will proceed to make similar changes to all other components that use OverlayTrigger, so all of them can benefit from the unbounded version. Once that work is done, I will change this PR to ready for review.

components/tooltip/Tooltip.razor.cs

site/AntBlazor.Docs/Demos/Components/Tooltip/demo/Basic.razor

anddrzejb · 2021-01-08T09:07:57Z

@ElderJames I am now working on Popover and I need advise. In the docs, Popover uses Button component as its child element. This is a component and not an element and it does not exposes its ref, so I cannot do

<Popover ContentTemplate="@_content" Title="Title">
    <Unbound>
        <Button type="primary" @ref="@context.Current">Hover me (unbound)</Button>
    </Unbound>
</Popover>

because I get a compilation error Cannot implicitly convert type 'AntDesign.Button' to 'Microsoft.AspNetCore.Components.ElementReference'. I cannot find any parameter in any of the base classes that would actually expose element reference to a dom element. Am I missing something?

ElderJames · 2021-01-08T09:25:16Z

In the end of the post of ForwardRef, gave a solution for components. It may be:

<Popover ContentTemplate="@_content" Title="Title">
    <Unbound>
        <Button type="primary" RefBack="@context" >Hover me (unbound)</Button>
    </Unbound>
</Popover>

anddrzejb · 2021-01-08T10:45:08Z

That unfortunately does not work. I actually tried that when I was going through what is available in the base classes (sorry for not communicating that clearly). It may have something to do with the fact that RefBack is not really attached to the top level dom element of the ChildContent. I will be trying to figure it out...

ElderJames · 2021-01-08T11:06:05Z

Maybe we need to see how does MatBlazor do that.

anddrzejb · 2021-01-08T11:13:29Z

I will try to work it out. For now I've done Dropdown and Popconfirm, but I imagine Popconfirm may have the same issue if used with a component, so this issue has to be solved first.
Also I was thinking - some components use Dropdown for their rendering (for example DatePicker). Should they be changed, so the (internally) will start using the <Unbound> child content?

ElderJames · 2021-01-08T11:55:48Z

Yes, we need to work on using <UnBound> for internal components after this PR is merged.

ElderJames · 2021-01-19T14:06:06Z

Yes, Column's tooltip should switch to unbound usage and make it align center.

anddrzejb · 2021-01-19T22:09:03Z

@ElderJames I think that my last commit 59aebd2 concludes the works within the scope of this PR (I've got to admit it was more work than I planned, but quite fruitful). With this commit, I finally solved the issue I had with AutoComplete. If you have anything else that you think should be done within this PR, please let me know. Otherwise I will soon change this draft into ready for review.

ElderJames · 2021-01-20T04:21:43Z

I think there are enough modifications to this pr that we can deal with it first.

components/upload/Upload.razor

ElderJames · 2021-01-20T15:34:07Z

components/upload/Upload.razor

                <Tooltip Title="tips" Disabled="file.State != UploadState.Fail" Class="@containerClass" Style="@(IsPictureCard ? "" : "display: list-item;")">
-                    @if (IsPictureCard && file.State == UploadState.Uploading)
-                    {
-                        <div class="ant-upload-list-item ant-upload-list-item-uploading ant-upload-list-item-list-type-picture-card">
-                            <div class="ant-upload-list-item-info">
-                                <div class="ant-upload-list-item-thumbnail">@Locale.Uploading</div>
-                                <span class="ant-upload-list-item-name ant-upload-list-item-name-icon-count-1" title="@file.FileName">
-                                    @file.FileName
-                                </span>
-                            </div>
-                            <div class="ant-upload-list-item-progress">
-                                <Progress Percent="file.Percent" StrokeWidth="2" Type="ProgressType.Line" ShowInfo="false"></Progress>
+                    <Unbound>


I find the Class in Unbound usage is not work, and here is missing the containerClass so the style is broken.

That is absolutely correct. This is the situation when Tooltip's div was actually used for something. Same situation as with Table's Column. I fixed it in the commit 9256ef1.

ElderJames · 2021-01-21T09:27:48Z

Finally we merged this PR, thanks a lot for this time!

anddrzejb · 2021-01-21T09:29:50Z

Thanks as well! I did not expect my attempt on making slider with tooltip will be so time consuming, but it was rewarding nevertheless.

ElderJames · 2021-01-21T09:45:53Z

Yes, you've solved a big problem!

@ref

* feat(module:overlay): OverlayTrigger not bound to a div * feat(module:overlay): OverlayTrigger not bound to a div * feat(module:overlay): Logic transfer to single Overlay * feat(module:overlay): remove obsolete duplication * feat(module:Tooltip): Add for unbounded oncontextmenu event handler * feat(module:tooltip): unbound js event listeners remove * docs(module:tooltip): unbound explanation * fix(module:button): attach Ref to top level html element @ref * feat(module:dropdown&tooltip&popconfirm&popover): Overlay not bound to a div * docs(module:dropdown&tooltip&popconfirm&popover): unbound explanation * feat(module:OverlayTrigger): common logic relocation * feat(module:overlaytrigger): Overlay not bound to a div * feat(module:DatePicker): Overlay not bound to a div * feat(module:select): Overlay not boud to div * fix(module:select): onclickarrow event relocation * fix(module:select): rename Show to OnArrowClick * feat(module:avatar): Overlay not bound to a div * docs(module:avatar): demo switch to unbound version * feat(module:autocomplete): partial OverlayTrigger not bound to a div * feat(module:slider): tooltip * docs(module:slider): tooltip * fix(module:overlay): add SetVisible method * feat: set Ref where missing, performance components register Ref when missing IsFixed flag for CascadeValue changed hard-code sequence numbers when using RenderTreeBuilder Rate component use Tooltip Unbound version Tabs test fix * fix: revert changes (accidental) * feat(module:upload): tooltip with unbound usage * feat(module:table): column use of unbound tooltip * feat(module:autocomplete):overlay unbound from div * fix(module:upload): missing div restore Co-authored-by: James Yeung <shunjiey@hotmail.com>

@ref

* feat(module:overlay): OverlayTrigger not bound to a div * feat(module:overlay): OverlayTrigger not bound to a div * feat(module:overlay): Logic transfer to single Overlay * feat(module:overlay): remove obsolete duplication * feat(module:Tooltip): Add for unbounded oncontextmenu event handler * feat(module:tooltip): unbound js event listeners remove * docs(module:tooltip): unbound explanation * fix(module:button): attach Ref to top level html element @ref * feat(module:dropdown&tooltip&popconfirm&popover): Overlay not bound to a div * docs(module:dropdown&tooltip&popconfirm&popover): unbound explanation * feat(module:OverlayTrigger): common logic relocation * feat(module:overlaytrigger): Overlay not bound to a div * feat(module:DatePicker): Overlay not bound to a div * feat(module:select): Overlay not boud to div * fix(module:select): onclickarrow event relocation * fix(module:select): rename Show to OnArrowClick * feat(module:avatar): Overlay not bound to a div * docs(module:avatar): demo switch to unbound version * feat(module:autocomplete): partial OverlayTrigger not bound to a div * feat(module:slider): tooltip * docs(module:slider): tooltip * fix(module:overlay): add SetVisible method * feat: set Ref where missing, performance components register Ref when missing IsFixed flag for CascadeValue changed hard-code sequence numbers when using RenderTreeBuilder Rate component use Tooltip Unbound version Tabs test fix * fix: revert changes (accidental) * feat(module:upload): tooltip with unbound usage * feat(module:table): column use of unbound tooltip * feat(module:autocomplete):overlay unbound from div * fix(module:upload): missing div restore Co-authored-by: James Yeung <shunjiey@hotmail.com>

feat(module:overlay): OverlayTrigger not bound to a div

ea297d1

pr-triage bot added the PR: draft label Dec 29, 2020

anddrzejb added 4 commits December 29, 2020 22:23

feat(module:overlay): OverlayTrigger not bound to a div

7b02c88

feat(module:overlay): Logic transfer to single Overlay

8c39956

merge with upstream

cf205e1

feat(module:overlay): remove obsolete duplication

6e2d872

Merge branch 'master' into overlayTriggerDiv

627b230

anddrzejb added 2 commits December 30, 2020 11:16

feat(module:Tooltip): Add for unbounded oncontextmenu event handler

f02a1a2

Merge branch 'overlayTriggerDiv' of https://github.com/anddrzejb/ant-…

8037db6

…design-blazor into overlayTriggerDiv

anddrzejb mentioned this pull request Jan 4, 2021

(Discussion needed) Should the wrapping div be removed from components using Overlay (like Tooltip)? #898

Closed

anddrzejb added 2 commits January 4, 2021 22:42

Merge branch 'master' into overlayTriggerDiv

371a8db

Merge branch 'master' into overlayTriggerDiv

a978e18

ElderJames reviewed Jan 7, 2021

View reviewed changes

components/tooltip/Tooltip.razor.cs Outdated Show resolved Hide resolved

ElderJames reviewed Jan 7, 2021

View reviewed changes

site/AntBlazor.Docs/Demos/Components/Tooltip/demo/Basic.razor Outdated Show resolved Hide resolved

anddrzejb added 2 commits January 7, 2021 09:32

feat(module:tooltip): unbound js event listeners remove

2c00885

docs(module:tooltip): unbound explanation

8eb8a04

anddrzejb added 2 commits January 10, 2021 19:04

Merge branch 'master' into overlayTriggerDiv

9a832cc

fix(module:button): attach Ref to top level html element @ref

a77b788

feat(module:upload): tooltip with unbound usage

e85c96e

anddrzejb added 2 commits January 19, 2021 15:15

feat(module:table): column use of unbound tooltip

ee4a5d5

feat(module:autocomplete):overlay unbound from div

59aebd2

anddrzejb marked this pull request as ready for review January 20, 2021 04:57

pr-triage bot added PR: unreviewed and removed PR: draft labels Jan 20, 2021

anddrzejb requested a review from ElderJames January 20, 2021 04:59

Merge branch 'master' into overlayTriggerDiv

05db03a

ElderJames reviewed Jan 20, 2021

View reviewed changes

anddrzejb added 2 commits January 20, 2021 17:14

Merge branch 'master' into overlayTriggerDiv

7e73e30

fix(module:upload): missing div restore

9256ef1

ElderJames approved these changes Jan 21, 2021

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Jan 21, 2021

Merge branch 'master' into overlayTriggerDiv

2ed7f27

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Jan 21, 2021

ElderJames merged commit 3116384 into ant-design-blazor:master Jan 21, 2021

pr-triage bot added PR: merged and removed PR: unreviewed labels Jan 21, 2021

anddrzejb deleted the overlayTriggerDiv branch January 21, 2021 09:24

ElderJames added this to the 0.6.0 - MILESTONE: January 2021 milestone Jan 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(module:overlay): OverlayTrigger not bound to a div #937

feat(module:overlay): OverlayTrigger not bound to a div #937

anddrzejb commented Dec 29, 2020 •

edited

codecov-io commented Dec 29, 2020 •

edited

ElderJames commented Dec 29, 2020

anddrzejb commented Dec 29, 2020

anddrzejb commented Dec 29, 2020 •

edited

ElderJames commented Dec 30, 2020

anddrzejb commented Dec 30, 2020

anddrzejb commented Jan 8, 2021

ElderJames commented Jan 8, 2021

anddrzejb commented Jan 8, 2021

ElderJames commented Jan 8, 2021

anddrzejb commented Jan 8, 2021

ElderJames commented Jan 8, 2021

ElderJames commented Jan 19, 2021

anddrzejb commented Jan 19, 2021

ElderJames commented Jan 20, 2021

ElderJames Jan 20, 2021

anddrzejb Jan 20, 2021

ElderJames commented Jan 21, 2021

anddrzejb commented Jan 21, 2021

ElderJames commented Jan 21, 2021

feat(module:overlay): OverlayTrigger not bound to a div #937

feat(module:overlay): OverlayTrigger not bound to a div #937

Conversation

anddrzejb commented Dec 29, 2020 • edited

🤔 This is a ...

🔗 Related issue link

💡 Background and solution

📝 Changelog

☑️ Self Check before Merge

codecov-io commented Dec 29, 2020 • edited

Codecov Report

ElderJames commented Dec 29, 2020

anddrzejb commented Dec 29, 2020

anddrzejb commented Dec 29, 2020 • edited

ElderJames commented Dec 30, 2020

anddrzejb commented Dec 30, 2020

anddrzejb commented Jan 8, 2021

ElderJames commented Jan 8, 2021

anddrzejb commented Jan 8, 2021

ElderJames commented Jan 8, 2021

anddrzejb commented Jan 8, 2021

ElderJames commented Jan 8, 2021

ElderJames commented Jan 19, 2021

anddrzejb commented Jan 19, 2021

ElderJames commented Jan 20, 2021

ElderJames Jan 20, 2021

Choose a reason for hiding this comment

anddrzejb Jan 20, 2021

Choose a reason for hiding this comment

ElderJames commented Jan 21, 2021

anddrzejb commented Jan 21, 2021

ElderJames commented Jan 21, 2021

anddrzejb commented Dec 29, 2020 •

edited

codecov-io commented Dec 29, 2020 •

edited

anddrzejb commented Dec 29, 2020 •

edited