Skip to content
This repository has been archived by the owner on Jul 19, 2024. It is now read-only.

PTL-19 Navigation Arrows #58

Merged
merged 10 commits into from
Mar 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 0 additions & 1 deletion src/components/AssetGrid/components/Asset/Asset.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ const Asset = ({image, assetType, typeIndex, imageEnv}: Props) => {
typeIndex,
heading: assetType.heading,
environment: env,
isError,
}
)

Expand Down
72 changes: 56 additions & 16 deletions src/components/AssetModal/AssetModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,27 @@ import Image from 'next/image'
import ArrowDownSvg from 'icons/svgs/arrow-down.svg'
import SearchWhiteSvg from 'icons/svgs/search-white.svg'
import DownloadSvg from 'icons/svgs/download.svg'
import {useAppSelector} from 'app/hooks'
import {useAppSelector, useAppDispatch} from 'app/hooks'
import {setSelectedAssetEnvironment} from 'features/planAssetsSlice'
import AssetErrorSVG from 'icons/svgs/asset-error.svg'
import downloadAsset from 'services/downloadAsset'
import {getSelectedAssetEnvironment, getSelectedAssetGroup} from 'features/planAssetsSlice'
import {classNames} from 'utils/classNames'
import {EnvironmentName, EnvironmentShortName} from 'utils/enums'


const AssetModal = () => {
const dispatch = useAppDispatch()
const [imageDimensions, setImageDimensions] = useState(null)
const [isError, setIsError] = useState(false)
const [isLoading, setIsLoading] = useState(true)
const imageClasses = imageDimensions ? 'opacity-100 transition-opacity duration-500' : 'opacity-0 transition-opacity'

const selectedAssetEnvironment = useAppSelector(getSelectedAssetEnvironment)
const selectedAssetGroup = useAppSelector(getSelectedAssetGroup)

const selectedAsset = selectedAssetGroup[selectedAssetEnvironment]
const {hasMultipleImagesOfThisType, typeIndex, image, heading, isError, environment} = selectedAsset
const {hasMultipleImagesOfThisType, typeIndex, image, heading, environment} = selectedAsset

Choose a reason for hiding this comment

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

Can the isError value on the PlanAsset type be removed completely now that this modal relies on it's own internal state to determine if there is an error or not?

Refer to line 36 in the Asset component.

const {id, url, description, encoding} = image

const urlArray = url.split('/')
Expand Down Expand Up @@ -57,9 +62,16 @@ const AssetModal = () => {
)


const renderAssetimage = () => {
const renderAssetImage = () => {

const handleOnLoadingComplete = (imageDimensions) => {
setImageDimensions(imageDimensions)
setIsLoading(false)
}
if (isError) {
return <AssetErrorSVG className='h-[22px] w-[22px]' />
return (
<AssetErrorSVG className='h-[22px] w-[22px]' />
)
}
return (
<Image
Expand All @@ -69,41 +81,69 @@ const AssetModal = () => {
height={imageDimensions?.naturalWeight || 280}
objectFit='contain'
alt={description || heading}
onLoadingComplete={(imageDimensions) => setImageDimensions(imageDimensions)}/>
onLoadingComplete={(imageDimensions) => handleOnLoadingComplete(imageDimensions)}
onError={() => setIsError(true)}/>
)
}

const renderImageSection = () => {
const isUniqueAcrossEnvironments = Object.values(selectedAssetGroup).filter(env => env?.image?.id).length === 1
const renderNavigationButton = rotation => (
const assetArray = Object.values(selectedAssetGroup)
const isUniqueAcrossEnvironments = assetArray.filter(asset => asset !== null).length === 1
enum NavigationDirection {

Choose a reason for hiding this comment

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

I would probably just leave these as plain strings, so as to not over engineer

Copy link
Author

Choose a reason for hiding this comment

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

I think it maybe easier to read as the Enums vs the classname it relates to but its not a big deal so changed it, see what you think :)

LEFT = 'rotate-90',
RIGHT = '-rotate-90'
}

const handleNavigationButtonClick = navigationDirection => {

Choose a reason for hiding this comment

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

These nested functions should really be prepended with an underscore, since they are technically private functions that can not be accessed outside of the scope of the parent function.

const _handleNavigationButtonClick

Copy link
Author

Choose a reason for hiding this comment

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

I didn't think this was a a common pattern in JS? I went down a rabbit hole of researching this its up for debate... I think it being a const with its normal const scoping rules is sufficient imo:

airbnb/javascript#490
https://eslint.org/docs/rules/no-underscore-dangle.html

If oyu feel strongly this should be a thing though, happy to do it.

const currentAssetIndex = assetArray.findIndex(asset => asset.environment === selectedAssetEnvironment)
const lastAssetIndex = assetArray.length - 1

let newEnvironment
if (navigationDirection === NavigationDirection.LEFT) {
newEnvironment = currentAssetIndex === 0 ? assetArray[lastAssetIndex].environment : assetArray[currentAssetIndex - 1].environment
} else {
newEnvironment = currentAssetIndex === lastAssetIndex ? assetArray[0].environment : assetArray[currentAssetIndex + 1].environment
}
dispatch(setSelectedAssetEnvironment(newEnvironment))
setIsError(false)
setIsLoading(true)
}

const renderNavigationButton = navigationDirection => (
<Button
handleClick={() => console.log('clicked')} // TODO: Placeholder for future ticket
handleClick={() => handleNavigationButtonClick(navigationDirection)} // TODO: Placeholder for future ticket
buttonWidth={Button.buttonWidth.TINY}
buttonSize={Button.buttonSize.TINY}
buttonBackground={Button.buttonBackground.BLUE}
labelColour={Button.labelColour.WHITE}
>
<ArrowDownSvg fill='white' className={`${rotation} scale-75`} />
<ArrowDownSvg fill='white' className={`${navigationDirection} scale-75`} />
</Button>
)

return (
<div className='w-full h-[280px] flex mb-[12px]'>
<div className='w-[50px] h-full flex items-center'>
{!isUniqueAcrossEnvironments && renderNavigationButton('rotate-90')}
{!isUniqueAcrossEnvironments && renderNavigationButton(NavigationDirection.LEFT)}
</div>
<div className='w-full h-full flex justify-center items-center'>
{renderAssetimage()}
{renderAssetImage()}
</div>
<div className='w-[50px] h-full flex justify-end items-center'>
{!isUniqueAcrossEnvironments && renderNavigationButton('-rotate-90')}
{!isUniqueAcrossEnvironments && renderNavigationButton(NavigationDirection.RIGHT)}
</div>
</div>
) }


const renderAssetDetails = () => {
if (isError || isLoading) {
return (
<div className='mb-[12px] min-h-[76px]'></div>
)
}
return (
<div className='mb-[12px]'>
<div className='mb-[12px] min-h-[76px]'>
<div className='flex justify-between mb-[2px]'>
<span className='font-heading-7'>Dimensions</span>
<span className='font-body-3'>{imageDimensions && `${imageDimensions.naturalWidth} x ${imageDimensions.naturalHeight}`}</span>
Expand All @@ -117,7 +157,8 @@ const AssetModal = () => {
<span className='font-body-3'>{filename}</span>
</div>
</div>
) }
)
}


const renderJSONSection = () => {
Expand All @@ -143,7 +184,6 @@ const AssetModal = () => {
{renderJson()}
</div>
</pre>

</div>
)
}
Expand Down Expand Up @@ -179,7 +219,7 @@ const AssetModal = () => {
<Modal modalHeader={`${heading} ${hasMultipleImagesOfThisType ? typeIndex + 1 : ''} Asset ${id}${isError ? ' could not load' : ''}`}>
{renderEnvironmentTags()}
{renderImageSection()}
{!isError && renderAssetDetails()}
{renderAssetDetails()}
{renderJSONSection()}
{renderButtons()}
</Modal>
Expand Down
1 change: 0 additions & 1 deletion src/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ export type PlanAsset = { // Plan Image with additional metadata used for Asset
hasMultipleImagesOfThisType: boolean
typeIndex: number,
heading: string,
isError: boolean,
environment: string,
}

Expand Down