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

[feat] make existent props reactive and new whileElementsMount prop #1

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

renatodeleao
Copy link

@renatodeleao renatodeleao commented Sep 22, 2022

Hey 👋 nicely done with the conversion and the video walkthrough!

I'm not sure if you did it just for the video /fun or if you have any intentions of maintaining this package. If that so, I would be interested to help. It's a shame that Vue does not have a first-class citizen wrapper for this package and the alternatives are a bit bloated instead of keeping the low-level premise of the original package.

Changes

  • chore updated dev dependencies audit warnings from
  • chore upgrades [floating-ui/dom] to latest version, which didn't break much things only an export rename and Adds autoUpdate util.
  • fix makes props reactive:
    • Still questioning if we should mutate the provided refs. Example, we pass a Ref for placement with the value of right, but since we have the flip() modifier the computed placement will actually be left, so toggling it to "right" on the consumer will actually result in it being mutated to left. Happy to hear your input on it.
  • feature: add whileElementsMounted prop matching the react package

Typescript

I'm not very proficient in typescript (as you can see by the code), so I would also use this as a learning experience. The types need to be corrected before building this fixes and features.

Future

Cheers!

@iamandrewluca
Copy link
Member

iamandrewluca commented Sep 25, 2022

Hey, @renatodeleao thanks for the feature! Pushed a commit with types improvements.

I think I will need to do a major version release because we updated @floating-ui/dom to next major version.

Still questioning if we should mutate the provided refs. Example, we pass a Ref for placement with the value of right, but since we have the flip() modifier the computed placement will actually be left, so toggling it to "right" on the consumer will actually result in it being mutated to left. Happy to hear your input on it.

I would like to keep the same behaviour as the react-dom package, if they do so, we should also.

@renatodeleao
Copy link
Author

renatodeleao commented Sep 26, 2022

Hey Andrew, thanks for the type fixes (and linting)!

I think I will need to do a major version release because we updated @floating-ui/dom to next major version.

If we're strict about it, yeah because that getScrollParents rename can be considered a breaking change 021184d
We could get away with it if this was in pre-release 1.0.0-beta.0 😛, but don't sweat about it major bump it is!

I would like to keep the same behaviour as the react-dom package, if they do so, we should also.

Got it, I don't think they mutate them. They seem to assign them to the "data" object initially and then simply replace that object with computePosition result, keeping original "refs" intact and then spreading that object out in the return value. We can follow a similar approach. They do mutate the the dataRef — from useRef(data) — but it's used only internally and they never expose it to consumers, probably for optimising some re-renders.

1. Make sure that computePosition does not mutate original prop refs, even if react to it
1. returns computedData as refs that react the computePosition. This aligns the code better with what we have in the original package.  As side-effect drop the _variable namespace in favour of a reactive object.
@renatodeleao
Copy link
Author

@iamandrewluca aligned the return values with the original package and the computePosition no longer mutates original props refs, while still reacting to their changes. Let me know if you spot anything missing. Cheers!

Comment on lines +70 to +71
placement: unref(placement),
strategy: unref(strategy),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reactive conversion is "deep": it affects all nested properties. A reactive object also deeply unwraps any properties that are refs while maintaining reactivity.
https://vuejs.org/api/reactivity-core.html#reactive

unref can be omitted

Copy link
Author

@renatodeleao renatodeleao Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the key part is "unwraps while maintaining reactivity", we don't want to keep reactivity or it would mutate original props refs as before. We just want o use its value like an initial value. EDIT: Also, since we can provide static strings as props, unref does the isRef(prop) ? prop.value : prop for us.

Codesandbox

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

2 participants