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.

Sign up for GitHub

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

Removed Edit Ability for Non-owned sketches #2904

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Removed Edit Ability for Non-owned sketches #2904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
580d121
Removed the ability to edit w/o duplicate.
vivekbopaliya Jan 15, 2024
d857bc6
Removed a few unused codes
vivekbopaliya Jan 15, 2024
8486392
Support for multiple languages
vivekbopaliya Jan 15, 2024
5c4875f
updated few unit test through npm test -- -u
vivekbopaliya Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions client/modules/IDE/components/Editor/MobileEditor.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export const EditorContainer = styled.div`

export const EditorHolder = styled.div`
min-height: 100%;
pointer-events: ${(props) => (props.readOnly ? 'none' : '')};
`;

export const PreviewWrapper = styled.div`
Expand Down
21 changes: 19 additions & 2 deletions client/modules/IDE/components/Editor/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import { CSSLint } from 'csslint';
import { HTMLHint } from 'htmlhint';
import classNames from 'classnames';
import { debounce } from 'lodash';
import { connect } from 'react-redux';
import { connect, useSelector } from 'react-redux';
import { bindActionCreators } from 'redux';
import MediaQuery from 'react-responsive';
import '../../../../utils/htmlmixed';
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

'editor-holder--readonly':
// 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
: ''
Comment on lines +511 to +515
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

});

return (
Expand Down Expand Up @@ -567,6 +574,12 @@ class Editor extends React.Component {
</header>
<section>
<EditorHolder
readOnly={
this.props.project.owner &&
this.props.user.username &&
this.props.project.owner.username !==
this.props.user.username
}
ref={(element) => {
this.codemirrorContainer = element;
}}
Expand All @@ -588,6 +601,10 @@ class Editor extends React.Component {
}

Editor.propTypes = {
// eslint-disable-next-line react/forbid-prop-types
user: PropTypes.object.isRequired,
// eslint-disable-next-line react/forbid-prop-types
project: PropTypes.object.isRequired,
Comment on lines +604 to +607
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

autocloseBracketsQuotes: PropTypes.bool.isRequired,
autocompleteHinter: PropTypes.bool.isRequired,
lineNumbers: PropTypes.bool.isRequired,
Expand Down
50 changes: 42 additions & 8 deletions client/modules/IDE/components/Header/MobileNav.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import { setLanguage } from '../../actions/preferences';
import Overlay from '../../../App/components/Overlay';
import ProjectName from './ProjectName';
import CollectionCreate from '../../../User/components/CollectionCreate';
import { cloneProject } from '../../actions/project';
import EditIcon from '../../../../images/pencil.svg';

const Nav = styled(NavBar)`
background: ${prop('MobilePanel.default.background')};
Expand Down Expand Up @@ -67,15 +69,20 @@ const LogoContainer = styled.div`

const Title = styled.div`
display: flex;
flex-direction: column;
gap: ${remSize(2)};
gap: ${remSize(10)};

* {
padding: 0;
margin: 0;
}

> h5 {
p {
margin-left: 2px;
padding: 3px 8px;
}

h5 {
margin-top: 2px;
font-size: ${remSize(13)};
font-weight: normal;
}
Expand Down Expand Up @@ -205,6 +212,8 @@ const MobileNav = () => {
const user = useSelector((state) => state.user);

const { t } = useTranslation();
const theme = useSelector((state) => state.preferences.theme);
console.log(theme);

const editorLink = useSelector(selectSketchPath);
const pageName = useWhatPage();
Expand All @@ -228,18 +237,43 @@ const MobileNav = () => {
}

const title = useMemo(resolveTitle, [pageName, project.name]);

const dispatch = useDispatch();
const Logo = AsteriskIcon;
return (
<Nav>
<LogoContainer>
<Logo />
</LogoContainer>
<Title>
<h1>{title === project.name ? <ProjectName /> : title}</h1>
{project?.owner && title === project.name && (
<h5>by {project?.owner?.username}</h5>
)}
<div>
<h1>{title === project.name ? <ProjectName /> : title}</h1>
{project?.owner && title === project.name && (
<h5>by {project?.owner?.username}</h5>
)}
</div>

<div>
{title === project.name &&
project.owner &&
user.username !== project.owner.username && (
// eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions
<p
Comment on lines +259 to +260
Copy link
Collaborator

Choose a reason for hiding this comment

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

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>.

className={`toolbar__duplicatetoedit ${
theme === 'contrast' ? 'contrast' : 'normal'
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}`}
onClick={() => dispatch(cloneProject())}
>
{t('Toolbar.DuplicateToEdit')}{' '}
<EditIcon
className={`toolbar__icon ${
theme === 'contrast' ? 'contrast' : 'normal'
}`}
focusable="false"
aria-hidden="true"
/>
</p>
)}
</div>
</Title>
{/* check if the user is in login page */}
{pageName === 'login' || pageName === 'signup' ? (
Expand Down
53 changes: 38 additions & 15 deletions client/modules/IDE/components/Header/Toolbar.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,18 @@ import PlayIcon from '../../../../images/play.svg';
import StopIcon from '../../../../images/stop.svg';
import PreferencesIcon from '../../../../images/preferences.svg';
import ProjectName from './ProjectName';
import { cloneProject } from '../../actions/project';
import EditIcon from '../../../../images/pencil.svg';

const Toolbar = (props) => {
const { isPlaying, infiniteLoop, preferencesIsVisible } = useSelector(
(state) => state.ide
);
const user = useSelector((state) => state.user);
const project = useSelector((state) => state.project);
const theme = useSelector((state) => state.preferences.theme);
const autorefresh = useSelector((state) => state.preferences.autorefresh);
const dispatch = useDispatch();

const { t } = useTranslation();

const playButtonClass = classNames({
Expand Down Expand Up @@ -97,20 +100,40 @@ const Toolbar = (props) => {
</label>
</div>
<div className="toolbar__project-name-container">
<ProjectName />
{(() => {
if (project.owner) {
return (
<p className="toolbar__project-project.owner">
{t('Toolbar.By')}{' '}
<Link to={`/${project.owner.username}/sketches`}>
{project.owner.username}
</Link>
</p>
);
}
return null;
})()}
<div className="toolbar__projectname">
<ProjectName />
{(() => {
if (project.owner) {
return (
<p className="toolbar__project-project.owner">
{t('Toolbar.By')}{' '}
<Link to={`/${project.owner.username}/sketches`}>
{project.owner.username}
</Link>
</p>
);
}
return null;
})()}
</div>
{project.owner && user.username !== project.owner.username && (
// eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions
<p
Comment on lines +120 to +121
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be a <button> too.

className={`toolbar__duplicatetoedit ${
theme === 'contrast' ? 'contrast' : 'normal'
}`}
onClick={() => dispatch(cloneProject())}
>
{t('Toolbar.DuplicateToEdit')}{' '}
<EditIcon
className={`toolbar__icon ${
theme === 'contrast' ? 'contrast' : 'normal'
}`}
focusable="false"
aria-hidden="true"
/>
</p>
)}
</div>
<button
className={preferencesButtonClass}
Expand Down
Loading