Skip to content
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

Fresh table pilot #7103

Merged
merged 33 commits into from Nov 19, 2021
Merged

Fresh table pilot #7103

merged 33 commits into from Nov 19, 2021

Conversation

Twixes
Copy link
Collaborator

@Twixes Twixes commented Nov 12, 2021

Changes

As part of #6710, we'll be refreshing tables for a better UX and performance (especially resizable ones). This adds the new table component (based on designs by Chris) and implements it for the Feature flags page.

Before

Zrzut ekranu 2021-11-12 o 16 42 01

After (WIP)

Zrzut ekranu 2021-11-12 o 16 43 20

</tr>
</thead>
<tbody>
{dataSource.map((data, rowIndex) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

The after table is looking so much better. Nice job! One of the biggest problems with our older tables was that it would lag with many rows (i.e., events table). I don't know if you have this mind already, but we could probably make leaps in performance with some kind of virtualization here. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Definitely something to consider, will try some virtualization out in events/persons when I get to them!

@Twixes Twixes marked this pull request as ready for review November 17, 2021 14:01
@Twixes
Copy link
Collaborator Author

Twixes commented Nov 17, 2021

Refined the design with Chris and added features such as pagination and sorting following that, so this is now reviewable.

Desktop Mobile
Zrzut ekranu 2021-11-17 o 15 10 25 localhost_8000_feature_flags(iPhone 6_7_8) (3)

@mariusandra
Copy link
Collaborator

Didn't go deep yet, but the featureFlags.js cypress test fails (person.js seems to be a flake due to bad caching), and so does typescript.

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

  • It looks weird when we make parts of the table gray
    image
  • Can we add an avatar before the name?
  • It feels like the table is broken when I'm sorting by multiple columns. I'd assume every table header column I click removes the currently applied sort.

@mariusandra
Copy link
Collaborator

Had another look, overall awesome, though I'd still improve some things.

  1. Still think the "multi column sort" feature, while cool and practical for a poweruser, is rather unintuitive and makes the app more complex than it has to be.

Demonstration with rageclicks (my interpretation on how people who rageclick do their clicks):

2021-11-18 09 47 01

I think if we ever want a secondary sort, it's something you'd do in the context of filtering (show all flags by user X, and sort them alphabetically) and in this case the answer would be better filtering support.

  1. Related, the table is by default sorted by the "created" column, but there's no indicator to make me sure that's the case, instead of being a fluke.

2021-11-18 09 52 59

  1. This looks smooth!

2021-11-18 09 53 33

  1. The page loads super quick, which is awesome! Hurray, HTML, I guess :D

  2. Regarding pagination, this feels out of scope for the first PR, but could be useful to consider. Going back/forward natively in large paginated tables causes issues with scrolling, unless there's some connection to the URL.

Compare feature flags

2021-11-18 10 00 38

To saved insights:

2021-11-18 10 06 29

Saved insights (which does paginated API requests) solved all of this by putting the param in the URL. The global URL "scroll to top" logic and the browser's memory of things fix the pagination and scrolling issues nicely.

So, getting to the point: could we implement some native URL handling into the table? E.g. a param searchParam="page" that would wire it all up automatically? It could make a bunch of pages, that do not paginate API requests, really simple.

  1. Sorting by status doesn't do anything

2021-11-18 10 03 42

@mariusandra
Copy link
Collaborator

  1. One more thing. While this loading is great for saying "table loading", it's not good for table row status updates. I think these should happen within the spinner this time, as the indicator is even invisible in the case of a low row:

2021-11-18 10 15 27

@Twixes
Copy link
Collaborator Author

Twixes commented Nov 19, 2021

Alright:
2021-11-19 12 12 09

Made the following changes:

  • sorting is now a column at a time
  • there's a way to set default sorting
  • search params are now how the current page is stored, with support for multiple paginated tables on a single page
  • disabled flags don't get muted
  • when loading is in progress, there's an overlay blocking interaction (I tried position: sticky, which is normally great, but too tricky with tables; also tried the nice new Spinner component, but it wasn't significantly more visible, so left it out)

I also wasted an honestly inordinate amount of time yesterday thinking that surely TypeScript, being pretty smart, can do type inference on the column type and recognize that a column with dataIndex specified gets the record[dataIndex] value passed into the render as the first arg, while one without dataIndex can't get such an individual value. It turns out though that TypeScript is actually useless at heterogenous arrays (e.g. an array where all items are of some type TableColumn<X, Y>, but Y being different for each item and so having to be inferred individually – which is what columns look like), so… yeah, didn't work.

This was referenced Nov 19, 2021
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Looks good, just one final thing:

2021-11-19 16 21 52

I don't think the pagination should scroll.

@Twixes Twixes merged commit 2f1a6af into master Nov 19, 2021
@Twixes Twixes deleted the lemon-table branch November 19, 2021 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants