feat(vwc-popup): change anchorEl to anchorId

[rinaok]

Find anchor by Id instead of by Element.
Make the popup be open from the start.
Fix some bugs and refactor the code.

[github-actions]

🚀

Latest successful build of the PR deployed here.

🚀

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[YonatanKra]

Now I see why you have this delay.
It should be documented in the tests:
it("should delay first position by 300ms")
Or something like that. How did you get to 300ms BTW?

[rinaok]

@YonatanKra Having to set timeout, I chose 300 milliseconds because I saw FAST had a 300 millisecond delay for their tooltip to appear.
Would you suggest a time of less than 300 milliseconds?

[YonatanKra]

As I said, we can go with 300ms, it just needs to be documented (in tests) as suggested. Why not 100ms (which is the maximum time for user interaction response)? Would it work then?
I guess 300ms is a heuristic because other factors might delay the page layout calculation (like long running JS or longer running calculations before the component loaded).
So 100ms will not be enough.
By the way, we'd might want to add our own "ready" promise on this component and then use it in the tests. This might be a handy API.
I just don't feel good enough with arbitrary numbers going around.
In either case (arbitrary or not), we need to document this in the tests as this is part of the API exposed to the user (expect the component to finish rendering in 300ms).

[YonatanKra]

Thinking about it, I think we can "hide" this setTimeout behind an updateComplete message. Something like that in the firstUpdated function:

this.updateComplete = new Promise(res => setTimeout(() => {
      this.open = this.initialDisplayState;
			this.updatePosition();
			res();
}, this.DELAY));

I think that's the right syntax. Then the updateComplete will reflect the 300ms wait.

[YonatanKra]

This way, the 300ms becomes an implementation details and not an unexpected side effect.

[rinaok]

@YonatanKra updateComplete is read-only

[yinonov]

Correct me if i'm wrong @rinaok but the denounce is required for document to be ready. The popup component has no way of knowing that

[yinonov]

Will this be of any help?

https://toruskit.com/blog/how-to-get-element-bounds-without-reflow/

[YonatanKra]

@YonatanKra updateComplete is read-only

I did something with: _getUpdateComplete. Might help but seems an overkill.
If 300ms is a must then we need to document it in the tests (e.g. should run positionUpdate on load after 300ms delay).

[yinonov]

@rinaok if the intersection observer solves it, we may wanna create a single intersectionObserver instance and reuse it.
notice it is being used as a service here
https://github.com/microsoft/fast/blob/master/packages/web-components/fast-foundation/src/utilities/intersection-service.ts

we can actually might as well use that service 🤔

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication