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: 178 v distance to #220

Merged
merged 9 commits into from
Oct 7, 2023
Merged

feat: 178 v distance to #220

merged 9 commits into from
Oct 7, 2023

Conversation

JaimeTorrealba
Copy link
Member

@JaimeTorrealba JaimeTorrealba commented Sep 19, 2023

closes #178

@netlify
Copy link

netlify bot commented Sep 19, 2023

Deploy Preview for cientos-tresjs ready!

Name Link
🔨 Latest commit 75426e9
🔍 Latest deploy log https://app.netlify.com/sites/cientos-tresjs/deploys/652068c73e68c30008470a00
😎 Deploy Preview https://deploy-preview-220--cientos-tresjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JaimeTorrealba JaimeTorrealba marked this pull request as ready for review September 20, 2023 14:17
@JaimeTorrealba
Copy link
Member Author

JaimeTorrealba commented Oct 5, 2023

@andretchen0 I know if a lot to see, but if you find time, without rush please check this too

Also, if you have a better solution of how to show the distance will be appreciated, I'll just console.log it, because create a 3D text will be too much

Copy link
Contributor

@andretchen0 andretchen0 left a comment

Choose a reason for hiding this comment

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

Works as advertised. 👍

There a couple language changes to make in the docs. Nothing major.

Otherwise, I made some suggestions:

  • Based on the comments in the code, you were looking for a way to incorporate other displays. I made a suggestion for the API, but feel free to implement or not.
  • Currently, passing a null ref to v-distance-to throws an unhandled error. With Vue, a lot of times it's easier to start with a null ref in the setup. So, I'd prefer to have a guard in this component to do some type checking, then either fail silently orconsole.warn if the ref isn't an object with a position. But it's your call.

docs/guide/directives/v-distance-to.md Outdated Show resolved Hide resolved
docs/guide/directives/v-distance-to.md Outdated Show resolved Hide resolved
docs/guide/directives/v-distance-to.md Outdated Show resolved Hide resolved
docs/guide/directives/v-distance-to.md Outdated Show resolved Hide resolved
docs/guide/directives/v-distance-to.md Outdated Show resolved Hide resolved
src/core/directives/vDistanceTo.ts Outdated Show resolved Hide resolved
playground/src/pages/directives/vDistanceTo.vue Outdated Show resolved Hide resolved
src/core/directives/vDistanceTo.ts Show resolved Hide resolved
Copy link
Contributor

@andretchen0 andretchen0 left a comment

Choose a reason for hiding this comment

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

Looks good!

Just one more thing! 🤪

src/core/directives/vDistanceTo.ts Show resolved Hide resolved
@JaimeTorrealba
Copy link
Member Author

If I was better with Typescript I could definitely do something better, but for now I'll validate manually :D

I appreciate this feedbacks

@JaimeTorrealba
Copy link
Member Author

@andretchen0 I'll be on vacations from 14 - oct, to 04 nov

So I'll not make more PRs for now, please take care of cientos 🙏

Copy link
Contributor

@andretchen0 andretchen0 left a comment

Choose a reason for hiding this comment

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

Looks good!

@andretchen0
Copy link
Contributor

@andretchen0 I'll be on vacations from 14 - oct, to 04 nov

Sounds good. See you in november!

@alvarosabu
Copy link
Member

@andretchen0 Hey there! Thanks for taking care of reviewing, what do you think, ready to merge?

Copy link
Member

@alvarosabu alvarosabu left a comment

Choose a reason for hiding this comment

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

Looks pretty handy @JaimeTorrealba

@andretchen0
Copy link
Contributor

@andretchen0 Hey there! Thanks for taking care of reviewing, what do you think, ready to merge?

It's ready to go, I believe.

@alvarosabu alvarosabu merged commit d2e2521 into main Oct 7, 2023
6 checks passed
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.

V-directives in cientos proposal
3 participants