Prototype network/storage mapping builder (single-select source variant) #25
Conversation
|
🚀 Deployed Preview: http://konveyor-virt-ui-pr-25-preview.surge.sh ✨ |
mturley
force-pushed
the
mapping-builder-prototype-single-sources
branch
from
3ed5195
to
4ebaee3
Compare
| {builderItems.map((item, itemIndex) => { | ||
| let key = ''; | ||
| if (mappingType === MappingType.Network) { | ||
| const t = item.source as IVMwareNetwork | null; |
There was a problem hiding this comment.
👍 Very cool use of typescript type assertion.
There was a problem hiding this comment.
Honestly though I can't remember why I used the variable name t. I think it came from when these were keyed by unique targets. I'll change it.
There was a problem hiding this comment.
Actually, turns out I didn't need this at all. Both types of sources have an id property. I'll remove the whole condition
| options={options} | ||
| value={[selectedOption]} | ||
| onChange={(selection) => { | ||
| setSource((selection as OptionWithValue<MappingSource>).value); |
| if (mappingType === MappingType.Network) { | ||
| return (target as ICNVNetwork).name; | ||
| } | ||
| if (mappingType === MappingType.Storage) { | ||
| return target as string; | ||
| } | ||
| return ''; |
There was a problem hiding this comment.
I wonder if there is a cleaner way to code this? I have used a similar pattern in the past but tried recently to switch to ternary expressions for these. It would probably be overkill for a switch statement. This works fine but I am always wishing there was some sort of middle ground.
There was a problem hiding this comment.
Yeah, I was thinking of a ternary expression, but that requires choosing either network or storage as the condition and then having "other" be assumed to be the other. i.e. if it's a network mapping do this, else do this. I don't know if we'll ever need to add a third kind of mapping, but this allows for that. Could always refactor that later, so if you think it should be a ternary I'm fine with that.
There was a problem hiding this comment.
I am fine with if statements if there are more than 2 conditions. & maybe we switch to switch statements if there are more than 3. Its more of a gripe with JS than with your PR :)
| options={targetOptions} | ||
| value={[selectedOption]} | ||
| onChange={(selection) => { | ||
| setTarget((selection as OptionWithValue<MappingTarget>).value); |
There was a problem hiding this comment.
If I had proper generics support in SimpleSelect this wouldn't even be necessary 😄
| export const getSourceById = (sources: MappingSource[], id: string): MappingSource | undefined => | ||
| sources.find((source) => source.id === id); | ||
|
|
||
| export const getBuilderItemsFromMapping = ( |
There was a problem hiding this comment.
Not yet, but it can be used to pre-populate the state here from API mappings when we add edit functionality.
| import { IMappingBuilderItem } from './MappingBuilder'; | ||
| import { IVMwareProvider, ICNVProvider } from '@app/Providers/types'; | ||
|
|
||
| export const getSourceById = (sources: MappingSource[], id: string): MappingSource | undefined => |
There was a problem hiding this comment.
It's called in getBuilderItemsFromMapping. Maybe not necessary to export it.
|
Awesome! Thanks @ibolton336 |
Closes #7.
Based on #24, this converts the mapping arrow-box groups so that each maps only a single source to a single target, and the same target can be selected in more than one group. This addresses some feedback that @vconzola had about the multi-select source control being confusing. This also simplifies the state management a bit, since it is closer to the actual API data structure.
Only one of #24 or #25 should be merged, the other will be closed after we review the two designs.