# DMusic + Superyacht VFT Viewer Review

[https://github.com/NoumenaDigital/superyacht-nft-viewer](https://github.com/NoumenaDigital/superyacht-nft-viewer)

[https://github.com/NoumenaDigital/dmusic-nft-viewer](https://github.com/NoumenaDigital/dmusic-nft-viewer)

## Overview

Both apps are pretty much the exact same — but this document was written while reviewing Superyacht

**In code snippet suggestions `+` are for additions `-` are for deletions**

## The Not so Good 😣

- Highly recommend converting to TypeScript
- Setup docs could be improved — but booted up no problem
- Leftovers from CRA - code, config, docs & images
- `manifest.json`
- `logo192.png`
- `logo512.png`
- `README.md`
- No CSS reset
- No linter - recommend ESLint & prettier
- All styles are in one file - We recommend making this more modular by breaking up the Sass into smaller bits - usually this is done by component
- `import`/`export` setup could be improved with a few more directories and `index.js` files
- Also if the Sass is broken up by component a few `index.scss` imports wouldn't be a bad idea

For example:

    > ls components
     MyComponent/ index.ts index.sass
    > cd components/MyComponent
    > ls .
     MyComponent.jsx MyComponent.sass index.ts

- Recommend to use relative CSS units over absolute CSS units
- Declared but not used - address is not used

    // NFTList.js line 5
    let { chain, address } = useParams();

- Should be using semantic HTML

    <!-- App.js line 11 -->
    - <div className="app">
    + <main className="app">
    
    <!-- Header.js line 8-10 -->
    + <nav>
        <Link to="/">
          <LogoIcon logoClass="header-logo" />
        </Link>
    + </nav>

- Unnecessary use of `let` - never reassigned

    // SearchAddress.js line 7
    let navigate = useNavigate();
    
    // NFTDetails.js line 7
    let { chain, hash, id } = useParams();
    
    // NFTList.js line 5
    let { chain, address } = useParams();

- Unnecessary clean up function

    // NFTListPage.js line 26
    return () => {};
    
    // NFTListPage.js line 31
    return () => {};
    
    // NFTDetails.js line 15
    return () => {};

- Unnecessary comments - code could be self documenting

    // NFTListPage.js line 48-57
    - // loading state
    if (loading)
     return <span className="info-messages">Loading please wait...</span>;
    
    - // error state
    - if (error || !filteredItems || filteredItems.length === 0)
    + const isError = error || !filteredItems || filteredItems.length === 0;
    + if (isError)
        return <span className="info-messages">No NFTs on this address</span>;
    
    - // regular data workflow
    return <NFTList nfts={filteredItems} />;
    
    // NFTDetails.js
    - // loading state
    if (loading)
      return <span className="info-messages">Loading please wait...</span>;
    
    - // error state
    if (error)
      return <span className="info-messages">No NFT on this address</span>;
    
    - // regular data workflow
    return (

- Catch but no UI error handling

    // NFTDetails.js line 39-45 - will result in empty description in sad path
    const parsedDescription = (() => {
      try {
        return JSON.parse(description);
      } catch (err) {
        return {};
      }
    })();

- Could make better use of object destructuring of `details` in `<NFTList />`
- Code could me more modular
- `renderData` functions could be their own component
- store could have `slices/` and `selectors/` directories

    // NFTDetails.js line 22-26 - could be a helper func (difficult to test as is)
    const renderAddress = (address) => {
      return address.length > 8
        ? address.substring(0, 4) + "..." + address.substring(address.length - 4)
        : address;
    };

- Slice reducer could be using `extraReducers` and `createAsyncThunk` from RTK
- [https://redux-toolkit.js.org/api/createAsyncThunk](https://redux-toolkit.js.org/api/createAsyncThunk)
- Could use `await` in slice fetches
- I recommend moving over to `extraReducers` and `createAsyncThunk` from RTK but if you’re going to keep this approach I recommend making use of the `async`/`await` syntax
- Below shows a pattern in `detailsSlice.js` that also exists in `listSlice.js`

    // detailsSlice.js line 34-48
    export function fetchDetails(params) {
      return async (dispatch) => {
        dispatch(setLoading());
    -    api
    -     .get(
    -       `nft-viewer/api/v1/chains/${params.chain}/contracts/${params.hash}/nfts/${params.id}`
    -     )
    -     .then((response) => {
    -       dispatch(setDetails(response.data));
    -     })
    -     .catch(() => {
    -       dispatch(setError());
    -     });
    +     try {
    +       const { data } = await api.get(
    +       `nft-viewer/api/v1/chains/${params.chain}/contracts/${params.hash}/nfts/${params.id}`				
    +       )
    +
    +       dispatch(setDetails(data));
    +     } catch () {
    +       dispatch(setError())
    +     }
      };
    }

## The Really not so Good 😱

- `.env` and other config in `nomad/` pushed to the repo
- Create a `.env.example` and remove config from repo
- No tests
- Hacky page setup for `/`
- Create a `pages` directory

    // App.js line 15
    <Route path="/" element={<SearchAddress />} /> // wrap in a Home.js
    
    // SearchAddress.js line 32 - This could be handled in a Home.js page wrapper
    useEffect(() => {
        document.body.classList.add("home-page");
        return () => {
          document.body.classList.remove("home-page");
        };
      }, []);

- `index` as react key - can lead to issues

    // NFTDetails.js Line 46-55
    return attributes.map((attribute, index) => {
      return (
        parsedDescription[attribute] && (
          <div key={index}>
            <span className="attribute">{attribute}: </span>
            <span className="value">{parsedDescription[attribute]}</span>
          </div>
        )
      );
    });
    
    // NFTList.js lines 21-47
    {nfts.map((nft, index) => {
      return (
        <tr key={index}>
          <td>
            <img
              onError={addDefaultSrc}
              src={
                window.__RUNTIME_CONFIG__.REACT_APP_BASE_ENDPOINT +
                  "ipfs-store/api/v1/ipfs/" +
                   nft.ipfs_image_hash || `/album.png`
              }
              className="thumbnail"
              alt="thumbnail"
            />
          </td>
          <td>{nft.name}</td>
          <td>
            <Link
              to={`/chains/${chain}/contracts/${nft.contract_hash}/nfts/${nft.id}`}
              className="link"
            >
              VIEW
            </Link>
          </td>
        </tr>
      );
    })}