Skip to content

Update Collection View #3270

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

Merged
merged 3 commits into from
Nov 7, 2024
Merged

Update Collection View #3270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b629fd
pass username prop
raclim Nov 7, 2024
0595b6c
add removeFromCollection action
raclim Nov 7, 2024
35c8b4f
update selectors
raclim Nov 7, 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
20 changes: 8 additions & 12 deletions client/modules/User/components/Collection.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,14 @@ import ArrowDownIcon from '../../../images/sort-arrow-down.svg';
import CollectionMetadata from './CollectionMetadata';
import CollectionItemRow from './CollectionItemRow';

const Collection = ({ collectionId }) => {
const Collection = ({ collectionId, username }) => {
const { t } = useTranslation();
const dispatch = useDispatch();

const { user, collection, sorting, loading, username } = useSelector(
(state) => ({
user: state.user,
collection: getCollection(state, collectionId),
sorting: state.sorting,
loading: state.loading,
username: state.user.username
})
);
const user = useSelector((state) => state.user);
const collection = useSelector((state) => getCollection(state, collectionId));
const sorting = useSelector((state) => state.sorting);
const loading = useSelector((state) => state.loading);

useEffect(() => {
dispatch(CollectionsActions.getCollections(username));
Expand All @@ -34,7 +29,7 @@ const Collection = ({ collectionId }) => {

const isOwner = () =>
user != null &&
user.username &&
typeof user.username !== 'undefined' &&
collection?.owner?.username === user.username;
Comment on lines 31 to 33
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is definitely room to clean this up further.

The user != null check is unnecessary because user is state.user which is always an object even when there is no one logged in.

You are now passing username as a prop, which is the username of the collection owner, so you can use that instead of collection?.owner?.username. Using username has a few advantages, including that it won't be undefined while the collection is fetching so you get an accurate boolean during loading.

Additionally, username is never undefined. So there is no possibility that username === user.username when there is no logged in user (and user.username is undefined). So you don't need to explicitly check that there is a user.username. If username === user.username is true then there is a user logged in who is the the owner of this collection. If it is false then either there is no user logged in or the logged in user is not the owner.

So you can write const isOwner = () => username === user.username and it will have the same effect.

When I converted this component I wrote it as:

const currentUsername = useSelector((state) => state.user.username);
const isOwner = username === currentUsername;

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO it's important to use descriptive variable names like currentUsername and ownerUsername to avoid bugs like the one that you fixed here.

I know that I can be rude sometimes but I think you can understand my frustrations. Because I already converted this component over a year ago and it was cleanly written and did not have that sort of mistake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching these, there's probably still a lot of other areas this component could be improved here!

Since I wasn't able to get through all of your work and made some changes to these files that differed from yours, I do get your frustrations on how it probably feels like this could've been preventable. Although it'll be a lot of work since I didn't keep up with the PRs from back then, if you'd like to, we can get still try to get them in. I could be wrong, but I think most of them could probably be merged with some minimal changes except for a few (like the Common Table Components and the CodeMirror one, esp since it's part of the fellowship). I was also planning to upgrade the Node.js minor version in the near future, which I think might conflict with this one.

This is a bit of a tangent and definitely depends on the context, but I also personally feel that it's okay for code to end up a little messy and have mistakes sometimes! While I do think it's important to try not to make them, I don't want to view them negatively because I feel like they're a really normal part of any process, and in most cases, can usually be fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wanted to add that I meant the latter more as a point of conversation, rather than disagreeing!


const hasCollection = () => !!collection;
Expand Down Expand Up @@ -160,7 +155,8 @@ const Collection = ({ collectionId }) => {
};

Collection.propTypes = {
collectionId: PropTypes.string.isRequired
collectionId: PropTypes.string.isRequired,
username: PropTypes.string.isRequired
};

export default Collection;
18 changes: 7 additions & 11 deletions client/modules/User/components/CollectionItemRow.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,15 @@ import PropTypes from 'prop-types';
import React from 'react';
import { Link } from 'react-router-dom';
import { useTranslation } from 'react-i18next';
import { useDispatch } from 'react-redux';
import { removeFromCollection } from '../../IDE/actions/collections';
import dates from '../../../utils/formatDate';
import RemoveIcon from '../../../images/close.svg';

const CollectionItemRow = ({
collection,
item,
isOwner,
removeFromCollection
}) => {
const CollectionItemRow = ({ collection, item, isOwner }) => {
const { t } = useTranslation();
const dispatch = useDispatch();
const projectIsDeleted = item.isDeleted;

const handleSketchRemove = () => {
const name = projectIsDeleted ? 'deleted sketch' : item.project.name;

Expand All @@ -22,7 +19,7 @@ const CollectionItemRow = ({
t('Collection.DeleteFromCollection', { name_sketch: name })
)
) {
removeFromCollection(collection.id, item.projectId);
dispatch(removeFromCollection(collection.id, item.projectId));
}
};

Expand Down Expand Up @@ -75,10 +72,9 @@ CollectionItemRow.propTypes = {
user: PropTypes.shape({
username: PropTypes.string.isRequired
})
}).isRequired
})
}).isRequired,
isOwner: PropTypes.bool.isRequired,
removeFromCollection: PropTypes.func.isRequired
isOwner: PropTypes.bool.isRequired
};

export default CollectionItemRow;
Loading