Skip to content

React class to functional conversion - final #3231

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 11 commits into from
Oct 2, 2024
Merged

React class to functional conversion - final #3231

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
25d23db
rest of files for the class to functional conversion
nahbee10 Sep 11, 2024
5c424ed
fix connect import for redux
nahbee10 Sep 13, 2024
613bb01
Merge branch 'develop' into react-fc-conversion-4
nahbee10 Sep 13, 2024
4cd3e0e
fix test error
nahbee10 Sep 13, 2024
a466640
console height error ifx
nahbee10 Sep 16, 2024
9ed5799
fix test error
nahbee10 Sep 16, 2024
3ce1cb4
add stoppropagation
nahbee10 Sep 16, 2024
4ffc471
fix test errors in SketchList
khanniie Sep 20, 2024
cf00dc4
remove comments and use better react functions to memoize
nahbee10 Sep 25, 2024
d7fe122
final edits
nahbee10 Oct 2, 2024
c7a619c
fix collection list error
nahbee10 Oct 2, 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
216 changes: 46 additions & 170 deletions client/modules/IDE/components/AssetList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,187 +1,63 @@
import PropTypes from 'prop-types';
import React from 'react';
import { connect, useDispatch } from 'react-redux';
import { bindActionCreators } from 'redux';
import { Link } from 'react-router-dom';
import React, { useEffect } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import { Helmet } from 'react-helmet';
import prettyBytes from 'pretty-bytes';
import { useTranslation, withTranslation } from 'react-i18next';
import MenuItem from '../../../components/Dropdown/MenuItem';
import TableDropdown from '../../../components/Dropdown/TableDropdown';

import { useTranslation } from 'react-i18next';
import AssetListRow from './AssetListRow';
import Loader from '../../App/components/loader';
import { deleteAssetRequest } from '../actions/assets';
import * as AssetActions from '../actions/assets';

const AssetMenu = ({ item: asset }) => {
const AssetList = () => {
const { t } = useTranslation();

const dispatch = useDispatch();
const { username, assetList, loading } = useSelector((state) => ({
username: state.user.username,
assetList: state.assets.list,
loading: state.loading
}));

Comment on lines +12 to +16
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a big deal but FYI it's better to have separate useSelector calls for each piece of state that you read:

Call useSelector Multiple Times in Function Components

When retrieving data using the useSelector hook, prefer calling useSelector many times and retrieving smaller amounts of data, instead of having a single larger useSelector call that returns multiple results in an object. Unlike mapState, useSelector is not required to return an object, and having selectors read smaller values means it is less likely that a given state change will cause this component to render.

However, try to find an appropriate balance of granularity. If a single component does need all fields in a slice of the state , just write one useSelector that returns that whole slice instead of separate selectors for each individual field.

https://redux.js.org/style-guide/#call-useselector-multiple-times-in-function-components

const handleAssetDelete = () => {
const { key, name } = asset;
if (window.confirm(t('Common.DeleteConfirmation', { name }))) {
dispatch(deleteAssetRequest(key));
}
};

return (
<TableDropdown aria-label={t('AssetList.ToggleOpenCloseARIA')}>
<MenuItem onClick={handleAssetDelete}>{t('AssetList.Delete')}</MenuItem>
<MenuItem href={asset.url} target="_blank">
{t('AssetList.OpenNewTab')}
</MenuItem>
</TableDropdown>
);
};

AssetMenu.propTypes = {
item: PropTypes.shape({
key: PropTypes.string.isRequired,
url: PropTypes.string.isRequired,
name: PropTypes.string.isRequired
}).isRequired
};

const AssetListRowBase = ({ asset, username }) => (
<tr className="asset-table__row" key={asset.key}>
<th scope="row">
<a href={asset.url} target="_blank" rel="noopener noreferrer">
{asset.name}
</a>
</th>
<td>{prettyBytes(asset.size)}</td>
<td>
{asset.sketchId && (
<Link to={`/${username}/sketches/${asset.sketchId}`}>
{asset.sketchName}
</Link>
)}
</td>
<td className="asset-table__dropdown-column">
<AssetMenu item={asset} />
</td>
</tr>
);

AssetListRowBase.propTypes = {
asset: PropTypes.shape({
key: PropTypes.string.isRequired,
url: PropTypes.string.isRequired,
sketchId: PropTypes.string,
sketchName: PropTypes.string,
name: PropTypes.string.isRequired,
size: PropTypes.number.isRequired
}).isRequired,
username: PropTypes.string.isRequired
};

function mapStateToPropsAssetListRow(state) {
return {
username: state.user.username
};
}

function mapDispatchToPropsAssetListRow(dispatch) {
return bindActionCreators(AssetActions, dispatch);
}

const AssetListRow = connect(
mapStateToPropsAssetListRow,
mapDispatchToPropsAssetListRow
)(AssetListRowBase);

class AssetList extends React.Component {
constructor(props) {
super(props);
this.props.getAssets();
}

getAssetsTitle() {
return this.props.t('AssetList.Title');
}
useEffect(() => {
dispatch(AssetActions.getAssets());
}, [dispatch]);

Copy link
Collaborator

Choose a reason for hiding this comment

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

i feel like since this is from the constructor we probably don't need anything in the dependency array?

Copy link
Collaborator

Choose a reason for hiding this comment

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

dispatch never changes its value so it doesn't make a difference whether it's in the dependency array or not. The effect will run one time on mount either way. Personally I'm in the habit of including it to appease linting rules such as react-hooks/exhaustive-deps. But we don't have that rule enabled on this repo (at least not yet) because we violate it all the time.

hasAssets() {
return !this.props.loading && this.props.assetList.length > 0;
}
const hasAssets = () => !loading && assetList.length > 0;

renderLoader() {
if (this.props.loading) return <Loader />;
return null;
}
const renderLoader = () => (loading ? <Loader /> : null);

renderEmptyTable() {
if (!this.props.loading && this.props.assetList.length === 0) {
const renderEmptyTable = () => {
if (!loading && assetList.length === 0) {
return (
<p className="asset-table__empty">
{this.props.t('AssetList.NoUploadedAssets')}
</p>
<p className="asset-table__empty">{t('AssetList.NoUploadedAssets')}</p>
);
}
return null;
}

render() {
const { assetList, t } = this.props;
return (
<article className="asset-table-container">
<Helmet>
<title>{this.getAssetsTitle()}</title>
</Helmet>
{this.renderLoader()}
{this.renderEmptyTable()}
{this.hasAssets() && (
<table className="asset-table">
<thead>
<tr>
<th>{t('AssetList.HeaderName')}</th>
<th>{t('AssetList.HeaderSize')}</th>
<th>{t('AssetList.HeaderSketch')}</th>
<th scope="col"></th>
</tr>
</thead>
<tbody>
{assetList.map((asset) => (
<AssetListRow asset={asset} key={asset.key} t={t} />
))}
</tbody>
</table>
)}
</article>
);
}
}

AssetList.propTypes = {
user: PropTypes.shape({
username: PropTypes.string
}).isRequired,
assetList: PropTypes.arrayOf(
PropTypes.shape({
key: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
url: PropTypes.string.isRequired,
sketchName: PropTypes.string,
sketchId: PropTypes.string
})
).isRequired,
getAssets: PropTypes.func.isRequired,
loading: PropTypes.bool.isRequired,
t: PropTypes.func.isRequired
};

function mapStateToProps(state) {
return {
user: state.user,
assetList: state.assets.list,
loading: state.loading
};
}

function mapDispatchToProps(dispatch) {
return bindActionCreators(Object.assign({}, AssetActions), dispatch);
}
return (
<article className="asset-table-container">
<Helmet>
<title>{t('AssetList.Title')}</title>
</Helmet>
{renderLoader()}
{renderEmptyTable()}
{hasAssets() && (
<table className="asset-table">
<thead>
<tr>
<th>{t('AssetList.HeaderName')}</th>
<th>{t('AssetList.HeaderSize')}</th>
<th>{t('AssetList.HeaderSketch')}</th>
<th scope="col"></th>
</tr>
</thead>
<tbody>
{assetList.map((asset) => (
<AssetListRow asset={asset} key={asset.key} username={username} />
))}
</tbody>
</table>
)}
</article>
);
};

export default withTranslation()(
connect(mapStateToProps, mapDispatchToProps)(AssetList)
);
export default AssetList;
73 changes: 73 additions & 0 deletions client/modules/IDE/components/AssetListRow.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import PropTypes from 'prop-types';
import React from 'react';
import { Link } from 'react-router-dom';
import { useDispatch } from 'react-redux';
import { useTranslation } from 'react-i18next';
import prettyBytes from 'pretty-bytes';
import MenuItem from '../../../components/Dropdown/MenuItem';
import TableDropdown from '../../../components/Dropdown/TableDropdown';
import { deleteAssetRequest } from '../actions/assets';

const AssetMenu = ({ item: asset }) => {
const { t } = useTranslation();
const dispatch = useDispatch();

const handleAssetDelete = () => {
const { key, name } = asset;
if (window.confirm(t('Common.DeleteConfirmation', { name }))) {
dispatch(deleteAssetRequest(key));
}
};

return (
<TableDropdown aria-label={t('AssetList.ToggleOpenCloseARIA')}>
<MenuItem onClick={handleAssetDelete}>{t('AssetList.Delete')}</MenuItem>
<MenuItem href={asset.url} target="_blank">
{t('AssetList.OpenNewTab')}
</MenuItem>
</TableDropdown>
);
};

AssetMenu.propTypes = {
item: PropTypes.shape({
key: PropTypes.string.isRequired,
url: PropTypes.string.isRequired,
name: PropTypes.string.isRequired
}).isRequired
};

const AssetListRow = ({ asset, username }) => (
<tr className="asset-table__row" key={asset.key}>
<th scope="row">
<a href={asset.url} target="_blank" rel="noopener noreferrer">
{asset.name}
</a>
</th>
<td>{prettyBytes(asset.size)}</td>
<td>
{asset.sketchId && (
<Link to={`/${username}/sketches/${asset.sketchId}`}>
{asset.sketchName}
</Link>
)}
</td>
<td className="asset-table__dropdown-column">
<AssetMenu item={asset} />
</td>
</tr>
);

AssetListRow.propTypes = {
asset: PropTypes.shape({
key: PropTypes.string.isRequired,
url: PropTypes.string.isRequired,
sketchId: PropTypes.string,
sketchName: PropTypes.string,
name: PropTypes.string.isRequired,
size: PropTypes.number.isRequired
}).isRequired,
username: PropTypes.string.isRequired
};

export default AssetListRow;
Loading
Loading