PTL-19 Navigation Arrows #58
Conversation
| RIGHT = '-rotate-90' | ||
| } | ||
|
|
||
| const handleNavigationButtonClick = navigationDirection => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 firstAssetEnvironment = assetArray[0].environment | ||
| const lastAssetEnvironment = assetArray[assetArray.length - 1].environment | ||
| let newEnvironment = selectedAssetEnvironment |
There was a problem hiding this comment.
There is no benefit to giving the newEnvironment variable a default value equal to selectedAssetEnvironment since one of the conditions underneath will always be true, and so in the end, newEnvironment will never equal selectedAssetEnvironment.
There was a problem hiding this comment.
Sure, this was a fallback for an earlier implementation.
| let newEnvironment = selectedAssetEnvironment | ||
|
|
||
| if (navigationDirection === NavigationDirection.LEFT) { | ||
| newEnvironment = selectedAssetEnvironment === firstAssetEnvironment ? lastAssetEnvironment : assetArray[currentAssetIndex - 1].environment |
There was a problem hiding this comment.
I think I would do away with the firstAssetEnvironment and lastAssetEnvironment variables and just focus on the index moving positions.
newEnvironment = currentIndex === assetArray.length - 1 ? assetArray[0].environment : assetArray[currentAssetIndex + 1].environment
There was a problem hiding this comment.
I have reworked this slightly, I was trying to make the logic more parsable so I have mostly done what you suggested.
| const assetArray = Object.values(selectedAssetGroup) | ||
| const isUniqueAcrossEnvironments = assetArray.filter(asset => asset !== null).length === 1 | ||
|
|
||
| enum NavigationDirection { |
There was a problem hiding this comment.
I would probably just leave these as plain strings, so as to not over engineer
There was a problem hiding this comment.
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 :)
| 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 |
There was a problem hiding this comment.
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.
Add functionality to allow the navigation arrow buttons to work.
Not entirely sure on my use of Enum here but otherwise hopefully pretty straightforward.