Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Issue 314 - Hidden Columns #508

Merged
merged 41 commits into from
Aug 1, 2019
Merged

Issue 314 - Hidden Columns #508

merged 41 commits into from
Aug 1, 2019

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Jul 18, 2019

Hidden columns feature #314

Support to toggle columns visibility.

API modifications

  • added column.hideable: boolean | undefined
  • added hidden_columns: string[] | undefined

This proposed solution deviates from the one describe in the parent issue in a few ways.

  • hideable only supports boolean: supporting boolean[] makes it hard to give a consistent experience when toggling visibility while supporting making non-hideable but hidden columns visible if necessary. It also requires more complex UI interactions that are not obvious to define correctly.. also deviates from more simple column visibility ideas usually associated with spreadsheets
  • the on-hover UI that shows a dropdown / marker for hidden columns interacts badly with the existing actions inside the header cells

So,

  • columns can only be hidden one by one
  • with multiple header rows, the last row is the one with the hide action
  • making columns visible again is done through a dropdown / toggle columns button / menu that closes when the user clicks on something else than the menu itself

While not ideal this solution should be flexible enough to make it possible for us to improve upon while not limiting our options.


On a totally different note, with the addition of multiple actions in the header cells, the use of unicode characters is starting to be problematic as they hardly ever align correctly or feel consistent. With that in mind, introducing fortawesome, a JS / es6-aware (read: tree shaking) wrapper around Font-Awesome free icon library to be used for the actions.

image

One of the goal of a previous PR was to make it easy for users to override the default icon with an icon of their choice. This is still possible with the proposed solution, but the selector(s) will be slightly different. As (a) the modified code w/ customization support has not been release yet and (b) this must be done through unofficial css props or custom stylesheet, this seems fine.

Marc-André Rivet added 3 commits July 17, 2019 15:23
- hidden vs. visible columns props at sanitation
# Conflicts:
#	src/dash-table/components/FilterFactory.tsx
#	src/dash-table/components/HeaderFactory.tsx
#	src/dash-table/components/Table/props.ts
#	src/dash-table/derived/filter/map.ts
#	src/dash-table/derived/header/content.tsx
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 July 18, 2019 12:48 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 July 18, 2019 14:29 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 July 18, 2019 16:46 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 July 18, 2019 16:52 Inactive
@chriddyp chriddyp requested a deployment to dash-table-review-pr-508 July 18, 2019 18:11 Abandoned
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 July 22, 2019 21:37 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 July 23, 2019 16:16 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 July 23, 2019 16:24 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 July 23, 2019 17:24 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 July 23, 2019 19:20 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 July 23, 2019 19:39 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 July 23, 2019 20:45 Inactive
Clearable = 'clearable',
ClearableMerged = 'clearableMerged',
Actionable = 'actionable',
ActionableMerged = 'actionableMerged',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified clearable modes to display all actions instead

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 August 1, 2019 13:31 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 August 1, 2019 14:24 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 August 1, 2019 14:50 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 August 1, 2019 15:01 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 August 1, 2019 15:06 Inactive
@Marc-Andre-Rivet
Copy link
Contributor Author

hideable can be a bool or array of bools, plus (and all similar props get this option too) 'last' to only apply to the last header row.

Similar props are: clearable, deletable, hideable, renamable. All of them now have last and first.

  • Staying consistent with the above, removed the table-level hideable prop as it does not have an equivalent.

  • The toggle menu shows all available options that can be toggled on/off and displays them in order of column_index, then row_index.

- update docstrings
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 August 1, 2019 15:19 Inactive
}</label>
</div>)
};
}))).filter(i => !R.isNil(i)).sort((a, b) => a.j - b.j).sort((a, b) => a.i - b.i).map(a => a.component)}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit messy -- can rework if seen as problematic.

Generates a sparse component[][] containing the list of inputs but keeping their associated index, remove nil items and sort in reverse order of condition/priority (columns are more important than rows -- ordering is like a radix sort), retrieve the component from the wrapper.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.sort((a, b) => (a.i - b.i) || (a.j - b.j)) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. That should work too. Checking / updating.

@@ -275,6 +272,8 @@ export interface IProps {
fill_width?: boolean;
filter_query?: string;
filter_action?: TableAction;
hideable?: boolean | boolean[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing

@@ -321,6 +320,7 @@ interface IDefaultProps {
fill_width: boolean;
filter_query: string;
filter_action: TableAction;
hideable: boolean | boolean[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing

i === 0 :
typeof flag === 'boolean' ?
flag :
!!flag && flag[i];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This piece of logic is needed in lots of places now

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 August 1, 2019 15:31 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-508 August 1, 2019 16:14 Inactive
* which column header row to display the `rename` action button in by
* supplying an array of booleans.
* For example, `[true, false]` will display the `rename` action button
* on the first row, but not the second row.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we using Python (True) or JS (true) capitalization here? This inconsistency exists all sorts of places across our component repos, but IIRC we said we'd try to standardize on JS in the JS code, paving the way for language-dependent translation at some future time in the back-end codegen.

Also [false, ..., true] is ambiguous. Perhaps [false, ..., false, true]? That said I might just remove the shortcut sentence, as it's not a fixed shortcut (depends on # of headers) and I think it's already pretty clear without that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will improve consistency for the modified props and go the JS route. Good point for the array examples.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Last doc comment is nonblocking. 💃

- improve boolean array usage examples
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants