-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature refactor selected columns #614
Conversation
src/Client/MainComponents/Widgets.fs
Outdated
model.ProtocolState.TemplatesSelected | ||
|> List.map (fun template -> Array.init template.Table.Columns.Length (fun _ -> true)) | ||
|> Array.ofList | ||
let columns = Array.init model.ProtocolState.TemplatesSelected.Length (fun _ -> Set.empty<int>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also do double index. So:
let columns = Set.empty<int,int>
where first int is template index and second int is column index. This would avoid initiating anything that is not used.
In addition columns
might now be skippedColumns
if i understand this correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Updated naming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor changes requested, overall looks good. Update and lgtm
(if selectionInformation.SelectedColumns.Length > 0 then | ||
selectionInformation.SelectedColumns.[tableIndex].[columnIndex] | ||
(if not selectionInformation.DeSelectedColumns.IsEmpty then | ||
not (Set.contains (columnIndex, tableIndex) selectionInformation.DeSelectedColumns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduce to second check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/Client/MainComponents/Widgets.fs
Outdated
|> Array.ofList | ||
{fst importTypeStateData with SelectedColumns = columns} |> snd importTypeStateData | ||
//if model.ProtocolState.TemplatesSelected.Length > 0 && (fst importTypeStateData).DeSelectedColumns.Length = 0 then | ||
// let columns = Array.init model.ProtocolState.TemplatesSelected.Length (fun _ -> Set.empty<int>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
burn it 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
let selectedData = selectionInformation.SelectedColumns | ||
selectedData.[tableIndex].[columnIndex] <- b | ||
{selectionInformation with SelectedColumns = selectedData} |> setSelectedColumns) | ||
let selectedData = SelectiveImportModalState.updateDeSelectedColumns(selectionInformation.DeSelectedColumns, tableIndex, columnIndex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change to member for clarity
As this can be run on the Set<int,int> just make it a function toggleSetItem(tableIndex, columnIndex)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -37,14 +37,13 @@ type SelectiveImportModal = | |||
style.height(length.perc 100) | |||
] | |||
prop.isChecked | |||
(if selectionInformation.SelectedColumns.Length > 0 then | |||
selectionInformation.SelectedColumns.[tableIndex].[columnIndex] | |||
(if not selectionInformation.DeSelectedColumns.IsEmpty then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is "deselect" not "deSelect"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/Client/Types.fs
Outdated
TemplateName = None | ||
} | ||
|
||
static member updateDeSelectedColumns(deSelectedColumns: Set<int*int>, tableIndex: int, columnIndex: int) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename toggle...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/Client/Types.fs
Outdated
static member getDeSelectedTableColumns (deSelectedColumns: Set<int*int>, tableIndex: int) = | ||
deSelectedColumns | ||
|> List.ofSeq | ||
|> List.map (fun item -> if (fst item) = tableIndex then Some (snd item) else None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List.choose here, not in the line below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Closes Issue #613