Use different label for array/exclude widget#77296
Conversation
octref
force-pushed
the
pine/array-settings
branch
from
2c94c56 to
e5eabc5
Compare
|
@roblourens OK, think this is ready for review! |
| : [...template.context.value]; | ||
|
|
||
| // Add pattern | ||
| if (e.pattern && !e.originalPattern) { |
There was a problem hiding this comment.
I don't think we should use the term "pattern", that's from exclude patterns.
| } | ||
| // Delete pattern | ||
| else if (!e.pattern && e.originalPattern) { | ||
| const patternIndex = newValue.indexOf(e.originalPattern); |
There was a problem hiding this comment.
What if the same value appears multiple times in the list? If I delete the 3rd one it should always remove exactly that one.
There was a problem hiding this comment.
Maybe this event should be index based
| Integer = 'integer', | ||
| Number = 'number', | ||
| Boolean = 'boolean', | ||
| ListOfString = 'list-of-string', |
There was a problem hiding this comment.
I think this could still be array. I suggested list for the view which displays multiple data types, but I think the data type should still match the setting type which is "array"
|
Fixed all. Should be good to go. |
| } else if (this.setting.type === 'boolean') { | ||
| this.valueType = SettingValueType.Boolean; | ||
| } else if (this.setting.type === 'array' && this.setting.listItemType === 'string') { | ||
| this.valueType = SettingValueType.Array; |
There was a problem hiding this comment.
This could still be ArrayOfString or whatever, I was just pointing out 'list' vs 'array'. But it's up to you
|
|
||
| scope?: ConfigurationScope; | ||
| type?: string | string[]; | ||
| listItemType?: string; |
There was a problem hiding this comment.
I think this should also be 'array' to match the type name right?
2 participants
Fix #62440