Removed Edit Ability for Non-owned sketches #2904

vivekbopaliya · 2024-01-15T17:58:02Z

Fixes #870

Changes:

bandicam.2024-01-15.18-38-01-506.mp4

Mobile view:

I was planning to introduce a 'Remix' function as it was talked about here then I realized that there's already a feature similar to 'Clone' (duplicate sketch) regardless you can still edit sketches that aren't yours (you can't add files/folders, though). To make it easier for users that help them distinguish between owned and non-owned sketches, we could make it necessary to duplicate sketches before making changes if they don't own them.

I have verified that this pull request:

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

lindapaiste

I'm not sure about this feature because we have it tagged as a high priority and something that we definitely want to do but I personally don't really like it 🤷‍♀️. I think it's nice to be able to edit a line or two in someone's code and see the changes without having to clone the whole thing. @raclim what are your thoughts?

There's a lot of cleanup before this could be considered for merging. But before you work on any of that, let's hear from @raclim about how we want this to function.

lindapaiste · 2024-01-15T18:44:29Z

client/modules/IDE/components/Editor/index.jsx

@@ -505,7 +505,14 @@ class Editor extends React.Component {
    const editorHolderClass = classNames({
      'editor-holder': true,
      'editor-holder--hidden':
-        this.props.file.fileType === 'folder' || this.props.file.url
+        this.props.file.fileType === 'folder' || this.props.file.url,
+      // eslint-disable-next-line no-dupe-keys


I don't think there's any dupe keys? You can probably delete this disable.

lindapaiste · 2024-01-15T18:47:13Z

client/modules/IDE/components/Editor/index.jsx

+        //  Check if there is a project owner, the user has a username,
+        // and the project owner's username is not the same as the user's username
+        this.props.project.owner && this.props.user.username
+          ? this.props.project.owner?.username !== this.props.user.username
+          : ''


We'll want to store the isReadOnly (or canEdit) condition to a variable because it is used in multiple places. But I don't think this logic is 100% correct because it would allow editing someone else's saved sketch if the user is not logged in.

lindapaiste · 2024-01-15T18:50:12Z

client/modules/IDE/components/Editor/index.jsx

+  // eslint-disable-next-line react/forbid-prop-types
+  user: PropTypes.object.isRequired,
+  // eslint-disable-next-line react/forbid-prop-types
+  project: PropTypes.object.isRequired,


IMO the better thing to do here would be to determine the isReadOnly/canEdit condition in a Redux selector. For right now we would add it to mapStateToProps and eventually when we re-write this component we can use the same selector function in a useSelector. That way we are only adding one new prop with a boolean value instead of passing down these complex objects.

lindapaiste · 2024-01-15T18:51:57Z

client/modules/IDE/components/Header/MobileNav.jsx

+              // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions
+              <p
+                className={`toolbar__duplicatetoedit ${
+                  theme === 'contrast' ? 'contrast' : 'normal'


You shouldn't need to access the theme directly here. You should use theme variables in the .scss instead. I can explain that more later if we decide to work with this PR.

lindapaiste · 2024-01-15T18:53:38Z

client/modules/IDE/components/Header/MobileNav.jsx

+              // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions
+              <p


The reason that you are getting all of these eslint errors is because you are using a <p> element instead of <button>. Keyboards, screen readers, etc. will not handle it properly unless it is a <button>.

lindapaiste · 2024-01-15T18:54:38Z

client/modules/IDE/components/Header/Toolbar.jsx

+          // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions
+          <p


This needs to be a <button> too.

lindapaiste · 2024-01-15T18:59:56Z

client/modules/IDE/pages/IDEView.jsx

+                      user={user}
+                      project={project}


See comments re: Editor.propTypes. I think that the condition which you are using currently is the equivalent of the isUserOwner variable here. Could be something like <Editor canEdit={isUserOwner} ...

lindapaiste · 2024-01-15T19:01:31Z

client/styles/components/_toolbar.scss

+    &.contrast {
+        background-color: #F5DC23;
+        color: black;
+    }
+
+    &.normal {
+        background-color: $p5js-pink;
+        color: white;
+    }


See comments re theme in components. We want this to look like color: getThemifyVariable('secondary-text-color'); (with whatever the correct variable name is).

vivekbopaliya · 2024-01-16T12:30:45Z

I'm not sure about this feature because we have it tagged as a high priority and something that we definitely want to do but I personally don't really like it 🤷‍♀️. I think it's nice to be able to edit a line or two in someone's code and see the changes without having to clone the whole thing. @raclim what are your thoughts?

There's a lot of cleanup before this could be considered for merging. But before you work on any of that, let's hear from @raclim about how we want this to function.

Sure, i'll be on it right after we hear from @raclim, thank you for all the above quality inputs though. (always appreciate them haha). Moreover, "I think it's nice to be able to edit a line or two in someone's code and see the changes without having to clone the whole thing." i think one of the primary reason that we are implementing this feature is to help users distinguish between their and others sketches as it is easy to get confused here. let's see how we are tackling this one!

raclim · 2024-01-16T20:20:29Z

I'm not sure about this feature because we have it tagged as a high priority and something that we definitely want to do but I personally don't really like it 🤷‍♀️. I think it's nice to be able to edit a line or two in someone's code and see the changes without having to clone the whole thing. @raclim what are your thoughts?

i think one of the primary reason that we are implementing this feature is to help users #870 (comment) their and others sketches as it is easy to get confused here. let's see how we are tackling this one!

Now that I'm looking at this I'm honestly not too sure!

I don't think I've personally had issues with the current setup, but can see how it can be hard to distinguish whether the sketch is yours or not. I'm down to implement something that signals to a user that they're working on a sketch that isn't their's more prominently, but I think I also feel hesitant about removing the edit ability altogether.

This is straying from the original issue and is just an idea, but I think one way "removing the edit ability" could be implemented as well might be to add "Set as Read Only" or "Set as Un-copyable" options for an individual sketch, so a user can choose if they don't want their sketches to be interacted with.

vivekbopaliya · 2024-01-17T16:41:29Z

I'm not sure about this feature because we have it tagged as a high priority and something that we definitely want to do but I personally don't really like it 🤷‍♀️. I think it's nice to be able to edit a line or two in someone's code and see the changes without having to clone the whole thing. @raclim what are your thoughts?

i think one of the primary reason that we are implementing this feature is to help users #870 (comment) their and others sketches as it is easy to get confused here. let's see how we are tackling this one!

Now that I'm looking at this I'm honestly not too sure!

I don't think I've personally had issues with the current setup, but can see how it can be hard to distinguish whether the sketch is yours or not. I'm down to implement something that signals to a user that they're working on a sketch that isn't their's more prominently, but I think I also feel hesitant about removing the edit ability altogether.

This is straying from the original issue and is just an idea, but I think one way "removing the edit ability" could be implemented as well might be to add "Set as Read Only" or "Set as Un-copyable" options for an individual sketch, so a user can choose if they don't want their sketches to be interacted with.

"I'm down to implement something that signals to a user that they're working on a sketch that isn't their's more prominently"
i think this can be tackled with simple "You're working on someone's sketch" the first time the user edits a sketch.
might be to add "Set as Read Only" or "Set as Un-copyable" options for an individual sketch
However, this doesn't completely solve the issue, as there will be even more scenarios for users to get confused about if we don't acknowledge them that they are trying to edit a non-owned, read-only sketch.

I believe these are two different features, so there must be two different approaches for each individually:

End the confusion between owned and non-owned sketches (show a toast on edit or something).
Letting users make their sketch read-only (this time we actually remove the ability to edit and duplicate non-owned sketches).

vivekbopaliya · 2024-01-25T11:10:11Z

This is straying from the original issue and is just an idea, but I think one way "removing the edit ability" could be implemented as well might be to add "Set as Read Only" or "Set as Un-copyable" options for an individual sketch, so a user can choose if they don't want their sketches to be interacted with.

the idea similar to this has already been discussed here. This can address both issues: hiding the open API keys as well as removing the ability to edit sketches if they are private or set to read-only. A week ago, I wasn't in favor of letting users keep their sketches as read-only because it seemed to lead to even more confusion and lots of work that might not be worth it?! However, now it appears to be more user-friendly than simply taking away the authority to edit every sketch haha. I am down to close this pull request and create a new one!

@raclim @catarak let me know how you want this to function.

raclim · 2024-01-25T21:23:29Z

Sorry about that, I definitely was proposing two different features! 😅

End the confusion between owned and non-owned sketches (show a toast on edit or something).

I think I'm personally leaning towards implementing this one regardless because for me, it feels like the simplest way to address the issue of a user having difficulty distinguishing whether a sketch is theirs or not. Down to rethink some of this though if this doesn't feel like enough!

Letting users make their sketch read-only (this time we actually remove the ability to edit and duplicate non-owned sketches).

I did stray from the original issue here—I proposed this as a way to implement this feature (removing the edit ability) in a way that might feel like a choice for the user if still we did want to integrate this into the editor.

the idea similar to this has already been discussed #1094 (comment). This can address both issues: hiding the open API keys as well as removing the ability to edit sketches if they are private or set to read-only.

I've actually been hoping to implement the privating sketches feature in the near future!

@vivekbopaliya These are all my thoughts, but I'm also wondering what makes sense to you here as well!

vivekbopaliya · 2024-01-26T07:49:47Z

Sorry about that, I definitely was proposing two different features! 😅

End the confusion between owned and non-owned sketches (show a toast on edit or something).

I think I'm personally leaning towards implementing this one regardless because for me, it feels like the simplest way to address the issue of a user having difficulty distinguishing whether a sketch is theirs or not. Down to rethink some of this though if this doesn't feel like enough!

Letting users make their sketch read-only (this time we actually remove the ability to edit and duplicate non-owned sketches).

I did stray from the original issue here—I proposed this as a way to implement this feature (removing the edit ability) in a way that might feel like a choice for the user if still we did want to integrate this into the editor.

the idea similar to this has already been discussed #1094 (comment). This can address both issues: hiding the open API keys as well as removing the ability to edit sketches if they are private or set to read-only.

I've actually been hoping to implement the privating sketches feature in the near future!

@vivekbopaliya These are all my thoughts, but I'm also wondering what makes sense to you here as well!

yes right! imo there are two ways to approach first proposal, which of one is similar to codepen's apporach.

In the current behavior, users can freely experiment with unowned sketches without duplication. The alternative way to duplicate a sketch is to simply save it (Ctrl + S), and it will be duplicated.
what we can do here is, for unowned sketches, we can display a small label (kinda similar to the above PR one) but without 'to edit,' simply saying Duplicate Sketch or Clone Original Sketch This way, without restricting editing ability, we can caution the user that they are working on someone else's sketch. A label with a background color always attracts eyes' attention.

along with that, on the user's first edit, we can show a toast message that goes something like 'You are working on a non-owned sketch; you might want to duplicate it first.' However, personally, this feels a little spoon-feeding lol.

this was for the first proposal only. once we resolve this, afterwards we can move on to the private sketch feature and discuss how we will want it to function.

vivekbopaliya added 4 commits January 15, 2024 19:15

Removed the ability to edit w/o duplicate.

580d121

Removed a few unused codes

d857bc6

Support for multiple languages

8486392

updated few unit test through npm test -- -u

5c4875f

lindapaiste requested changes Jan 15, 2024

View reviewed changes

lindapaiste added Status: Needs Design Enhancement Area:Editor For CodeMirror-related features labels Jan 15, 2024

vivekbopaliya mentioned this pull request Feb 18, 2024

Private Sketch feat. #3034

Open

4 tasks

lindapaiste mentioned this pull request Feb 26, 2024

Logged out error modal pops up when downloading another user's sketch #3053

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed Edit Ability for Non-owned sketches #2904

Removed Edit Ability for Non-owned sketches #2904

vivekbopaliya commented Jan 15, 2024

lindapaiste left a comment

lindapaiste Jan 15, 2024

lindapaiste Jan 15, 2024

lindapaiste Jan 15, 2024

lindapaiste Jan 15, 2024

lindapaiste Jan 15, 2024

lindapaiste Jan 15, 2024

lindapaiste Jan 15, 2024

lindapaiste Jan 15, 2024

vivekbopaliya commented Jan 16, 2024

raclim commented Jan 16, 2024

vivekbopaliya commented Jan 17, 2024

vivekbopaliya commented Jan 25, 2024

raclim commented Jan 25, 2024

vivekbopaliya commented Jan 26, 2024

		// eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions
		<p

Removed Edit Ability for Non-owned sketches #2904

Are you sure you want to change the base?

Removed Edit Ability for Non-owned sketches #2904

Conversation

vivekbopaliya commented Jan 15, 2024

Mobile view:

lindapaiste left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivekbopaliya commented Jan 16, 2024

raclim commented Jan 16, 2024

vivekbopaliya commented Jan 17, 2024

vivekbopaliya commented Jan 25, 2024

raclim commented Jan 25, 2024

vivekbopaliya commented Jan 26, 2024