p5.js-web-editor
p5.js-web-editor copied to clipboard
Private Sketch feat.
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
developbranch. - [x] is descriptively named and links to an issue number, i.e.
Fixes #123
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?
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.
@lindapaiste @raclim still waiting for UI to be approved so that i can go ahead
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!
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.
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 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!
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
@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.
Sounds good, I'll also do some further testing on my end as well!
Nice work! I hope we'll see this public soon as it will also fix issues with private api keys being publicly exposed.
@raclim it's been a quite time. What's status of this feature?
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!
No worries at all! Thanks for the update. happy to help whenever I can.
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?
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.
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?
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:
-
Saving a new sketch.
-
Making the sketch private by toggling the "private" checkbox.
-
Setting the sketch back to public by toggling the setting in My Sketches.
-
Navigate back to the sketch, make some changes, and then attempt to save. On save, I get the following error.
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?
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 /.
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!
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."
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:
Proposed View with Dropdown:
@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!
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:
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)?
Let me know your thoughts!
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)?
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.
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!
Let me know your thoughts!