p5.js-web-editor icon indicating copy to clipboard operation
p5.js-web-editor copied to clipboard

Private Sketch feat.

Open vivekbopaliya opened this issue 1 year ago • 22 comments

Changes:

https://github.com/processing/p5.js-web-editor/assets/122638553/40cf3f6f-15af-43b6-9226-de5783b43ff0

WORK FLOW:

  • Every sketch is default public, user can change to Private.
  • Private sketch will be visible to owner of project only.
  • If someone who is not owner tries to access Private project through URL, they will be redirected to /
  • Filtering is done on backend.

I have verified that this pull request:

  • [x] has no linting errors (npm run lint)
  • [x] has no test errors (npm run test)
  • [x] is from a uniquely-named feature branch and is up to date with the develop branch.
  • [x] is descriptively named and links to an issue number, i.e. Fixes #123

vivekbopaliya avatar Feb 18 '24 09:02 vivekbopaliya

This is awesome. I can tell that you have put a lot of thought into it and done some very good work here! I'm sure @raclim will have some thoughts on the UI. I'm going to see if I can figure out why you're getting errors in the getProjectsForUser handler. I'm not sure what steps we need to take as far as backwards compatibility, as existing projects won't have a visibility property. Possibly it's enough just to set default: 'Public' on the Schema?

lindapaiste avatar Feb 18 '24 21:02 lindapaiste

I'm not sure what steps we need to take as far as backwards compatibility, as existing projects won't have a visibility property. Possibly it's enough just to set default: 'Public' on the Schema?

Yeah, that will work. will do that. However, I have set up the state like this in Toolbar.jsx:

const [visibility, setVisibility] = React.useState(
    project.visibility || 'Public'
);

So for old sketches, the default value "Undefined" will be replaced by "Public" in the state. This way, the user should be able to change the visibility of old sketches as well.

vivekbopaliya avatar Feb 19 '24 02:02 vivekbopaliya

@lindapaiste @raclim still waiting for UI to be approved so that i can go ahead

vivekbopaliya avatar Mar 08 '24 04:03 vivekbopaliya

Thanks so much for working on this and I'm sorry for the delay in getting to this!

Honestly, this looks really awesome and I don't think I have much else to add so far on the UI! I know there were some previous designs done for a potential Privacy feature. It's a bit late, but you can probably still choose to incorporate them if you'd like, but I think what's currently here is great as well.

I think one potential next step for this after some further development is having it tested out with a small group of users who can probably give further feedback from there on the design/functionality as well. I can also try to start tagging and sharing it with some folks if it makes more sense to do it sooner rather than later down the line!

raclim avatar Mar 18 '24 16:03 raclim

I know there were some previous designs done for a potential Privacy feature.

This whole look is quite so cool and refreshing actually. I'm excited about adding it to my design. It's got a premium vibe that I think will work really well. However, it seems like it will require a significant amount of effort on its own. I'm currently tied up with some other tasks for the time being, but I'll try to dedicate some time to it within the next month and raise a PR. In the meantime, It would be best to focus on its functionality and gathering feedback related to it. it's better to move forward by sharing it with some individuals to gather feedback on its functionality.

vivekbopaliya avatar Mar 18 '24 16:03 vivekbopaliya

It's a bit late, but you can probably still choose to incorporate them if you'd like, but I think what's currently here is great as well.

@raclim Just a quick update: I've created a new PR with changes inspired by the design. let me know your thoughts!

vivekbopaliya avatar May 04 '24 08:05 vivekbopaliya

@vivekbopaliya Sorry that it took a while, just want to update that I created a shareable environment for this PR! Please feel free to share this around and get some feedback on it!

I feel like it looks pretty solid so far though, and super excited to see this out soon!

raclim avatar Jun 13 '24 20:06 raclim

Release Environments

This Environment is provided by Release, learn more! To see the status of the Environment click on Environment Status below.

:wrench:Environment Status : https://app.release.com/public/Processing%20Foundation/env-6ee1c7dda9

  • p5.js-web-editor

:wrench:Environment Status : https://app.release.com/public/Processing%20Foundation/env-adee4fd928

release-com[bot] avatar Jun 14 '24 03:06 release-com[bot]

@vivekbopaliya Sorry that it took a while, just want to update that I created a shareable environment for this PR! Please feel free to share this around and get some feedback on it!

I feel like it looks pretty solid so far though, and super excited to see this out soon!

Finally, it is here! Thank you very much. Let's see how this works out. If it does, we might want to design an implementation for mobile view as well.

vivekbopaliya avatar Jun 14 '24 06:06 vivekbopaliya

Sounds good, I'll also do some further testing on my end as well!

raclim avatar Jun 14 '24 12:06 raclim

Nice work! I hope we'll see this public soon as it will also fix issues with private api keys being publicly exposed.

Felixpaulsen avatar Aug 01 '24 18:08 Felixpaulsen

@raclim it's been a quite time. What's status of this feature?

vivekbopaliya avatar Jan 06 '25 06:01 vivekbopaliya

Hi @vivekbopaliya, thanks so much for checking back in on this. I genuinely appreciate your patience and I'm really sorry about not following through with an update sooner. I got caught up with a few tasks that came up and wasn't able to stay on top of this, but will be more responsive here moving forward.

To give more context on what's happening here, over the past few months we've been handling a wave of privacy-related requests from schools. We decided to prioritize receiving legal consultation before moving forward with any larger features, which included this one, to ensure we were adhering to best practices and regulations. It's taken longer than expected, but we're aiming to wrap up the consultation by the end of this month.

Once that's done, I think it makes sense to make this feature a priority. If you're still on board, we could aim to roll this out by the end of February or mid-March? Thank you so much again for your patience and for sticking around with this!

raclim avatar Jan 06 '25 14:01 raclim

No worries at all! Thanks for the update. happy to help whenever I can.

vivekbopaliya avatar Jan 15 '25 08:01 vivekbopaliya

Hi @vivekbopaliya! I'm sorry that it took a little longer, but we were finally able to finish our legal consultation, and confirmed that we're able to move forward with this feature as planned. Although it's been a while, would you still be open to completing it?

raclim avatar Mar 06 '25 19:03 raclim

Hi @vivekbopaliya! Just wanted to check in again if you'd be interested in furthering development on this. If not, I'd love to continue the development on this if it's alright with you.

raclim avatar Apr 10 '25 19:04 raclim

Hey @raclim thank you for getting back to me. Yes I'm still on this, and I'll try to finish the pending work in a few days.

Anything additional changes you need me to include?

vivekbopaliya avatar Apr 10 '25 20:04 vivekbopaliya

Thanks so much again for your patience and for checking back in on this after quite some time! Just wanted to note that there's no rush on trying to wrap it up, I just wanted to check in and see if you'd still be down!

I think the only change that seemed to be left was the one pointed out in this comment (https://github.com/processing/p5.js-web-editor/issues/1987#issuecomment-2166794463). I was able to recreate it by:

  1. Saving a new sketch. Screenshot 2025-04-10 at 4 36 53 PM

  2. Making the sketch private by toggling the "private" checkbox. Screenshot 2025-04-10 at 4 36 59 PM

  3. Setting the sketch back to public by toggling the setting in My Sketches. Screenshot 2025-04-10 at 4 37 12 PM

  4. Navigate back to the sketch, make some changes, and then attempt to save. On save, I get the following error. Screenshot 2025-04-10 at 4 37 29 PM

I think besides this, this feature might overall be good to try to release for now! I think I would just double check that private sketches are only visible to the project owners and other users who attempt to do so are redirected to /. I'm also just wondering, would this also guarantee that it's not searchable by the browser as well?

raclim avatar Apr 10 '25 20:04 raclim

hey @raclim, I've pushed new changes, also incorporated the old bug mentioned here: https://github.com/processing/p5.js-web-editor/pull/3108#issuecomment-2137933375. do check out!

to clarify, sketches are only visible to their project owners. If another user tries to access a private sketch, they’ll be redirected to /. However, it's worth noting that private sketches may still be discoverable via browser search, but surely any attempt to open them will result in a redirect to /.

vivekbopaliya avatar Apr 20 '25 05:04 vivekbopaliya

Just wanted to update that I recreated the testing environment for this PR! I'll look through it over the next few days, as well as with the PF team, and should have an update by the middle of next week!

raclim avatar May 08 '25 02:05 raclim

image

I’d suggest renaming the column to "Visibility" instead of "Make Private". That label reads more clearly as a status rather than an action, and it would also align better with the other column headers.

I’d also recommend using a dropdown with options like "Private" and "Public" instead of a toggle (even though it looks really nice). This would make the options more legible and give us more flexibility in the future if we want to support additional visibility states like "Unlisted."

SableRaf avatar Jun 05 '25 13:06 SableRaf

I’d also recommend using a dropdown with options like "Private" and "Public" instead of a toggle (even though it looks really nice). This would make the options more legible and give us more flexibility in the future if we want to support additional visibility states like "Unlisted."

Thanks so much @SableRaf for the suggestion! Would it look something like this? I can do some quick A/B Testing with Amy to see what folks think!

Current View with Toggle: Private_Feature_A

Proposed View with Dropdown: Private_Feature_B

Private_Feature_B_2

raclim avatar Jun 23 '25 17:06 raclim

@raclim Sure, I’ve pushed the fix. The issue was happening because the server was updating the updatedAt field, but the frontend (Redux store) didn’t have that updated value. So when we tried to update the sketch, the server threw a 409 Conflict error since it thought the data was outdated.

Also, I wanted to quickly bring up something for consistency:

Currently, on the collection page, we show non-owners the message: "Sketch is private" for private sketches. But on the sketch list, we don’t show private sketches at all to non-owners.

Should we align this behavior? Either:

hide private sketches everywhere for non-owners, or show them with a "Sketch is private" placeholder consistently.

Let me know your thoughts! Screenshot from 2025-07-05 13-48-42

vivekbopaliya avatar Jul 05 '25 08:07 vivekbopaliya

Thanks so much @SableRaf for the suggestion! Would it look something like this? I can do some quick A/B Testing with Amy to see what folks think!

Thanks! The dropdown looks good, but I think it should also indicate the current state (whether the sketch is Public or Private) without needing to click it. That way it’s immediately clear what the current visibility setting is.

Here's how YT does it for example:

image

SableRaf avatar Jul 07 '25 12:07 SableRaf

Hi @vivekbopaliya!

I wanted to check-in to see if it felt okay time-wise to transition from the current design of the privacy toggle to a dropdown one, similar to the one on Youtube proposed by @SableRaf. If it does feel okay, would it also be possible to also have the toolbar utilize the same dropdown design as well, replacing the checkbox (pictured below)?

Screenshot 2025-07-15 at 4 39 34 PM

Let me know your thoughts!

raclim avatar Jul 15 '25 20:07 raclim

Hi @vivekbopaliya!

I wanted to check-in to see if it felt okay time-wise to transition from the current design of the privacy toggle to a dropdown one, similar to the one on Youtube proposed by @SableRaf. If it does feel okay, would it also be possible to also have the toolbar utilize the same dropdown design as well, replacing the checkbox (pictured below)?

Screenshot 2025-07-15 at 4 39 34 PM Let me know your thoughts!

Sure! I'm almost done with it. just wrapping up a few things. I'll push the changes in the next couple of days.

vivekbopaliya avatar Jul 19 '25 13:07 vivekbopaliya

Thanks so much @vivekbopaliya for your updates on this!

Just wanted to note that after taking a look, I think the PR overall feels functionally good to me! We just merged in a few changes to update the mongoose and node version which seems like it's causing a minor merge conflict, but overall I think this feels great!

raclim avatar Jul 29 '25 18:07 raclim