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

Border colors 2 #424

Merged
merged 48 commits into from
May 10, 2019
Merged

Border colors 2 #424

merged 48 commits into from
May 10, 2019

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Apr 30, 2019

This is a continuation of the previous border colors PR. This takes the essential elements from the PR and while keeping the same underlying ideas that were developed there, makes the implementation a bit tighter.

  • use subset of border** CSS rules to style the table's borders (border, border-left, border-right, border-bottom, border-top) --> while only supporting a subset might feel limiting, it covers pretty much all cases with very little redundancy vs. the full rules set and simplifies implementation greatly
  • omit all border** CSS rules from general cell/data/headers/filter/ops styling
  • apply default styling
  • reconcile common borders between various types & with n_fixed_rows & n_fixed_columns (priority order: data > filter > headers > operations > fillers)

n_fixed_rows, n_fixed_columns cases are handled through CSS: https://github.com/plotly/dash-table/pull/424/files#diff-02b1c6154ad8311a2face953de21380dR159

headers, filters, data and operations are reconciled with priority

  1. data
  2. filters
  3. headers
  4. operations

Given that all the items above share style_cell and style_cell_conditional, and that in all cases these are the lowest weight styles applied,

p1 will win on p2 if:

  • p1's edge is not the table's default and not from style_cell or style_cell_conditional

  • p1's edge weight is greater than p2's edge weight -- the weight is equal to the source style's index

  • unit tests for edge matrices generation

  • visual tests for table rendering

Note: Percy diffs will be all over the place because of the pixel-differences between box-shadow and border rendering. We'll need to be careful with that one.

Marc-André Rivet added 2 commits April 30, 2019 13:23
- edges generation tests
- use in actual table
- default style
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 April 30, 2019 20:03 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 April 30, 2019 23:54 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 1, 2019 01:37 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 1, 2019 01:59 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 1, 2019 02:28 Inactive
- fix standalone filter tests
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 1, 2019 12:03 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 1, 2019 12:37 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 1, 2019 13:06 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 1, 2019 17:55 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 1, 2019 18:03 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 1, 2019 18:12 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 1, 2019 18:19 Inactive
@Marc-Andre-Rivet Marc-Andre-Rivet marked this pull request as ready for review May 1, 2019 18:32
@Marc-Andre-Rivet Marc-Andre-Rivet changed the title [WIP] Border colors 2 Border colors 2 May 1, 2019
export type RequiredProp<T, R extends keyof T> = T[R];
export type OptionalProp<T, R extends keyof T> = T[R] | undefined;

export type PropOf<T, R extends keyof T> = R;
Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet May 1, 2019

Choose a reason for hiding this comment

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

Just defining some types to do TypeScript stuff later..

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 8, 2019 20:39 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 8, 2019 21:09 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 8, 2019 22:53 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 8, 2019 23:54 Inactive
@Marc-Andre-Rivet
Copy link
Contributor Author

@alexcjohnson This is finally ready for another round. It's a bit of a mess by now though :(

@Marc-Andre-Rivet
Copy link
Contributor Author

Marc-Andre-Rivet commented May 9, 2019

Found more issues while demoing this to @cldougl

  • ops columns do not evaluate conditional styles correctly
  • non-border rules do not get applied on ops cells
  • op cells are misaligned in fixed sections with ☝️ applied
  • filter columns with invalid values get erased when a new valid value is entered
  • need to check out impact on select_cells highlight (this behaves as before)
  • changelog
  • need to do some code clean up
  • dash docs update

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 9, 2019 16:21 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 9, 2019 17:37 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 9, 2019 17:54 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 9, 2019 17:56 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 9, 2019 19:46 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 10, 2019 16:25 Inactive
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-424 May 10, 2019 17:00 Inactive
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.

Good grief, that was a heck of a lot bigger than it seemed it would be. I have no more comments to make, this is super thorough. 💃

@Marc-Andre-Rivet Marc-Andre-Rivet merged commit be5b38f into master May 10, 2019
@Marc-Andre-Rivet Marc-Andre-Rivet deleted the border-colors-2 branch May 10, 2019 19:52
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.

3 participants