Repository navigation
Conversation
|
@LFDanLu This is the PR to look at for floating-ui, although it also addresses various other issues, like the ones coming from |
| * Similar to `element.getBoundingClientRect()`, but intersected with all ancestors. | ||
| */ | ||
| public get visibleRect(): DOMRect { | ||
| throw new Error('Not implemented yet.'); |
There was a problem hiding this comment.
Kinda. I don't have a great use case for it yet, besides window scrolling in virtualizer, so it just satisfies the type for now. IIRC it wasn't added in virtualizer because walking the tree would be too costly.
|
@LFDanLu Is there anything left to do for me here? I saw the DNM branches got closed yesterday. I've been putting off work on test suites for this this PR and the scroll utilities PR, because they are quite a lot of work, and I haven't really gotten a definitive signal from you guys about whether to proceed here. |
|
@nwidynski heya, the team tested on Monday and had some comments that came out of that as you have seen already on the other branches. I would echo Rob's sentiment here, where the stories you provided for this branch performed well, but the team wasn't quite sure the best way to validate that everything worked together/what other areas this fixed since we don't have visual regression tests nor stories for the linked issues setup already. We'll try and set that up next time w/ regards to
100% agree, its definitely a ton of work to add all of this without knowing if it will be accepted and thank you for understanding our position where its hard to know whether we wanna accept something without that substantial work being done in the first place. Seeing as the goal of this is to eventually enable Navigation Component/Carousel and serve as an alternative for floating ui for overlay positioning, would it be helpful if we pointed these towards a feature branch instead of main so we can merge them together more easily and go from there? Trying to spitball how best to move forward here since this work will be long lived/have many moving parts and thus span multiple release cycles making it hard to merge things in confidently |
|
@LFDanLu I've just decided to stop complaining about it and get it over with 😅 The scroll utils PR now has a full browser test suite, on which we can expand for
I'm generally a bit hesitant towards that. Not because I don't agree with the logistics, but rather because I'm vary about stacking further PRs on top of something that I'm not sure will be accepted. Feature branches in my opinion only work if they receive the same review treatment as main, only with less requirement for polish. So I would basically have to be certain that once something lands on the feature branch, it at least is the right API/architecture, and will eventually go to main once polished & tested. Otherwise I risk building on top of unreviewed architecture decisions that render the whole feature branch useless in the end, even though there would have been mergable parts contained within. I hope that makes sense. |
|
@LFDanLu Okay, test suite for this PR is done also 👍 Migration will be done in follow-ups as mentioned, since they would blow up this PR. You can imagine |
|
hey thanks for looking into improvements here! I have been thinking about two approaches that could get us out of the business of overlay positioning entirely:
I wondered what you thought of those options. Do you think it is worth us maintaining a completely independent implementation now that browsers have a built-in solution? |
I believe there's been some misunderstanding. I think of
This PR solves A and B, while A)
As for my opinion on the two options, I'm a bit torn. On one hand, I think giving users options through abstracts like the recently added So while I think Option 1 can make sense, I would keep it internal, and then migrate |
|
I had Claude put together a demo of the |
|
I think the question is if we did outsource this to the browser (anchor positioning) or a third party library (floating-ui), do we still need to maintain these low level utilities ourselves? I would assume floating-ui has all of these already so they wouldn't depend on anything we provide. And of course the browser would handle all this itself as well. In my opinion, ideally the bundle size for overlay positioning would go to ~zero because we swapped to the built in browser primitive. Whether we can do that in a minor or a major is an open question though. |
Yes, that's the right thought. Architecturally, there are only two use-cases in which measuring layout is a thing: anchor positioning and scroll(IntoView). So your question brings us back to So yes, I think the primitives here still make sense at this point. That being said though, with |
Closes no issues directly, because this PR is intentionally limited to additions only. Migrating call sites to the new utilities and signatures is to be done in chore follow-ups, because it would otherwise be rather hard to review what changed here.
Disclaimer: This PR is tightly related to #10556! Unfortunately Github doesn't allow for Stacks to be built across forks, so we have to do with this :/
From a high-level, this PR fixes various issues with layout precision and lays the groundwork for an alternative to floating-ui. The primary file to look at is
layout.ts, which features a short description.Will close #7142, #10036, #10131, #9318 and more.
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: