Reviewed 21 of 22 files at r1.
Reviewable status: 21 of 22 files reviewed, 9 unresolved discussions (waiting on @j-low, @laurenbarker, @maryliag, and @nathanstilwell)

a discussion (no related file):
@maryliag ,tried to unselect "All" checkbox and it didn't work. I assume it should unselect all options and allow users to select particular columns.
Also, when I try manually unselect all options, clicking on the last option - toggles all options back to selected state.

packages/cluster-ui/src/columnsSelector/columnsSelector.tsx, line 42 at r1 (raw file):

  return (
    <components.Option {...props}>
      <input

Is it possible to reuse CheckboxInput component from ui-components package (packages/ui-components/src/Input/CheckboxInput.tsx)?

packages/cluster-ui/src/columnsSelector/columnsSelector.tsx, line 133 at r1 (raw file):

    allOptions: OptionsType<SelectOption>,
  ) => {
    let selected = selectedOptions

@maryliag , this logic with strings manipulations looks quite complex. Could you explain the main idea behind this?

packages/cluster-ui/src/columnsSelector/columnsSelector.module.scss, line 11 at r1 (raw file):

    height: 32px;
    width: 67px;
    font-size: 12px;

Default font-sizes and spacing can be reused from packages/cluster-ui/src/core/index.module.scss (.lavel class uses it already)

packages/cluster-ui/src/statementsPage/statementsPage.tsx, line 76 at r1 (raw file):

  totalFingerprints: number;
  lastReset: string;
  columns: string;

@maryliag , what is the main intend to store columns as a string? Would it be better to keep it in array of strings?

packages/cluster-ui/src/statementsPage/statementsPage.tsx, line 327 at r1 (raw file):

      this.state.tableColumnsSelected == undefined ||
      this.state.tableColumnsSelected.length == 0 ||
      (this.state.tableColumnsSelected == "default" &&

What "default" value means?

packages/cluster-ui/src/statementsPage/statementsPage.selectors.ts, line 97 at r1 (raw file):

      },
    );
    return Object.keys(databases);

nit. It might be more clear to rewrite it in more declarative way with filter + map functions and then get rid of duplicate values with Set.

return [
  ...new Set(statementsState.data.statements
    .filter(s => !!s.key.key_data.database)
    .map(s => s.key.key_data.database)
  ).values()
]
  

packages/cluster-ui/src/statementsTable/statementsTable.tsx, line 155 at r1 (raw file):

    displayColumns == "" ||
    displayColumns == "default"
  )

nit. wrap if statement in { ... } to comply with linting rules

packages/cluster-ui/src/statementsTable/statementsTable.tsx, line 159 at r1 (raw file):

  return columns.filter(option => {
    return displayColumns.split(",").includes(option.name);

it might be refactored out from filter predicate:

const displayCol = displayColumns.split(",");
return columns.filter(option => {
    return displayCol.includes(option.name);
}

Reviewable status: 15 of 23 files reviewed, 9 unresolved discussions (waiting on @j-low, @koorosh, @laurenbarker, and @nathanstilwell)

a discussion (no related file):

Good point, I forgot that case! It's fixed now!

packages/cluster-ui/src/columnsSelector/columnsSelector.tsx, line 42 at r1 (raw file):

Weird as it sounds the CheckboxInput is not really a checkbox, is just a regular input that I can then force to be checkbox. The problem is that it includes a wrapper div that I can't change the class and is messing up all alignments of the option, so I opted for a simple input instead.

packages/cluster-ui/src/columnsSelector/columnsSelector.tsx, line 133 at r1 (raw file):

I realized from your comment that was indeed confusing, so I restructured a little and used better names to help this issue. I also change to use array instead of string, so there is less manipulation. The idea here is: when a value is selected/deselected I don't have access to the value itself, only the array of the current selected ones, so what I'm doing is:

• check if the value "all" was the one clicked, if it was deselected, remove everything, if was selected, add everything
• if another value was selected and because of it all the values are now selected, update the "all" to be selected too
• otherwise just add/remove the value

Let me know if is better now :)

packages/cluster-ui/src/columnsSelector/columnsSelector.module.scss, line 11 at r1 (raw file):

Done.

packages/cluster-ui/src/statementsPage/statementsPage.tsx, line 76 at r1 (raw file):

I thought it would be better to save as string on cache to save space. I did refactor the code to get the value from cache (which is a string) and then transform to array. This way the code is easier to understand.

packages/cluster-ui/src/statementsPage/statementsPage.tsx, line 327 at r1 (raw file):

on the majority of cases "default" means select everything, but you can have some columns you want to ignore. For example, we don't want to show the column "database" by default, so if the value is "default" we will hide that column.

packages/cluster-ui/src/statementsPage/statementsPage.selectors.ts, line 97 at r1 (raw file):

Done.

packages/cluster-ui/src/statementsTable/statementsTable.tsx, line 155 at r1 (raw file):

Done.

packages/cluster-ui/src/statementsTable/statementsTable.tsx, line 159 at r1 (raw file):

Done.

Reviewed 2 of 8 files at r2, 11 of 11 files at r4.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @j-low, @koorosh, and @laurenbarker)