stefanobartoletti / nuxt-social-share

Simple Social Sharing for Nuxt 3
https://stefanobartoletti.github.io/nuxt-social-share/
MIT License
106 stars 8 forks source link

fix(props): reactive #159

Open aoor9 opened 2 months ago

aoor9 commented 2 months ago

Type of change

Description

I noticed that some prop values (eg. label, url etc.) are immutable, once set, which is frustrating. In some cases, I need to change those settings dynamically, especially when I globally share the component in my app. But any method I tried to update the needed props was pointless, as long as the component prevents this, and I see no reason why.

For example, the url prop, when undefined, it gets the URL from the browser and keeps that value until an app reload, which is not good.

Let's suppose I have the component inside app.vue

<template>
  <SocialShare
    network="facebook"
    styled="true"
    label="false"
    :url="currentUrl"
   />
</template>

<script setup lang="ts">
const currentUrl = ref<string | undefined>(undefined)
</script>

where url gets the app link with current path (so initializing it with undefined is fine), and I expect it to update its value whenever and wherever I need, like on page change event, from /index to /about.

If I try to change it manually currentUrl.value = "whatever url string I have in mind" or while watching the useRoute composable, the component gets unaffected.

Same for label (I fixed it first because it was easier), when I wanted it dynamically based on screen mode, like showing it on desktop and hiding it on mobile.

📝 Checklist

stefanobartoletti commented 2 months ago

Thanks for opening this, can you please provide some more context so I can better understand the use cases, and maybe a working example to test it myself?

Do you just need these values to be reactive?

Also, I see that there is an edit in https://github.com/stefanobartoletti/nuxt-social-share/pull/159/files#diff-c263b8042c6101dc0038fc1bf7e3d722ca34d14882a069fc53b8699d4b1a8400 that does not seem to be related.

aoor9 commented 2 months ago

@stefanobartoletti Updated the message and removed the edits unrelated to the issue.

stefanobartoletti commented 1 month ago

Thanks, it is a bit clearer now, but If you could provide a reproduction it would help to better place all these things in context and allow me to directly see and test it.

As a side note, it is always better to open issues to discuss the situation before opening a possibly breaking PR, it would make all the process smoother for everyone.

aoor9 commented 1 month ago

I hope this will be clear enough https://stackblitz.com/edit/nuxt-starter-wknavd

stefanobartoletti commented 1 month ago

Ok it is a bit clearer now.

It looks like it makes more sense for the URL though as it may not update the desired shared page, and in the current state seems to be a broken feature; dynamically toggling the label doesn't seem to be a feature worth including this way at the moment, since it will probably be reworked to allow dynamic text soon.

If you want to go on and implement the other property we can then review the changes together later.

Please also run eslint to check for errors and code styling issues.

aoor9 commented 1 month ago

I pushed another code change, for url option, and I don't know whether it's an optimal one, I spent 2 days to make it work (you can see the updated playground).

Please also run eslint to check for errors and code styling issues.

I'm linting the code, although there is no settings for VS Code. Making some and adapt the existing eslint rules to them, so you can lint the code on file save, might be less painful than running the command on terminal.

aoor9 commented 1 month ago

Can you please review it?

stefanobartoletti commented 1 month ago

Thanks for the contribution, but at a first glance I see far too many edits and unnecessary line breaks that change the formatting without actually altering the code.

In the current state, it is very difficult to tell apart new code from what is only a different formatting from the source codebase (that, to be honest, are not desired edits. The provided eslint config should be enough to fix things, it seems that some different rules than those already provided by this package were enforced).

Even some of the code changes do not seem to adhere to proper Nuxt coding standards (i.e., at a first glance, imports are being made from vue instead of #imports).

I hope I'm not sounding blunt, this is not my intention, I appreciate your effort in trying to contribute, but I also need to be open, this PR does not seem to be of a good enough quality at the moment to be considered for merging, and even the whole communication was not really that clear

There is a reason why open source projects have guidelines about contributing, like in example opening issues with a detailed explanation of the problem before pushing a PR, providing demos and reproductions with a clear context, and generally speaking adhering to standards already set up in the code base: it saves both time and possible frustrations on both parts and helps in having a successful outcome for contributions.

It is very difficult in the current state to properly evaluate what this PR is doing and how it is implementing the changes, and at the moment I'm quite busy with my work, I'm sorry but I cannot dedicate the necessary time and concentration to go through this now.