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

feat: Column list with usage column and new reusable table #684

Merged
merged 32 commits into from
Sep 24, 2020

Conversation

Golodhros
Copy link
Member

@Golodhros Golodhros commented Sep 23, 2020

Summary of Changes

Swaps the current Column list with the new reusable table
Adds the usage column
Updates the reusable table with new features
Updated some icon's greys

Fullscreen_9_23_20__4_58_PM

Components___Table_-_Customized_Table_⋅_Storybook

Tests

Added tests
Updated coverage settings

Documentation

Added table's stories

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes, including screenshots of any UI changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public python functions and the classes in the PR contain docstrings that explain what it does
  • PR passes all tests documented in the developer guide

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
@feng-tao
Copy link
Member

hey @Golodhros ,
I haven't gone through the code, so feel free to correct me if I miss anything. I have a few questions based on staging:

  1. should we put it in the feature branch given it is a bit large refactor?
  2. For those that don't have column usage, will they have two-column and the usage column will hide?
  3. We now add another row (column, type, usage) now, the column and columns seems a bit duplicate, should we rename that to name?
  4. Once I unclick, it will still show Column Statistics Stats reflect data collected between Aug 19, 2020 and Sep 18, 2020. but the column stats are no longer there.
  5. And once we add more column stats, will the style of stats change or the same as before?

Thanks, will look at more once fully back next week.

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
@Golodhros
Copy link
Member Author

Golodhros commented Sep 23, 2020

hey @Golodhros ,
I haven't gone through the code, so feel free to correct me if I miss anything. I have a few questions based on staging:

  1. should we put it in the feature branch given it is a bit large refactor?

Well, it is big indeed, but I am confident it won't change a lot of things as it is only adding the usage information to the list. I think we are ready to merge it.

  1. For those that don't have column usage, will they have two-column and the usage column will hide?

I synced with Daniel about this. We will be showing the usage column if any of the columns have usage information, but if none of them have it, we won't show the usage column.

  1. We now add another row (column, type, usage) now, the column and columns seems a bit duplicate, should we rename that to name?

Oh, I see. You mean with the name of the tab, right? Good call! I will ask Knowl, anyways it will be super easy to change.

  1. Once I unclick, it will still show Column Statistics Stats reflect data collected between Aug 19, 2020 and Sep 18, 2020. but the column stats are no longer there.

Yes, as the 'usage' data is always visible on the top row. Do you feel that's confusing? Maybe we could be more specific and say "Column usage reflects data..."

  1. And once we add more column stats, will the style of stats change or the same as before?

Open to discussion. I kept the old component around, so it would be easy to have that table back. I didn't love the styling of it though :)

Thanks, will look at more once fully back next week.

Thanks! I did a walkthrough with Allison today and we tweaked some stuff so we will merge it for now. Also Knowl has some styling tweaks that I will add on the next PR.

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Copy link
Contributor

@allisonsuarez allisonsuarez left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
@Golodhros Golodhros merged commit 050f674 into master Sep 24, 2020
@ttannis ttannis deleted the mi-using-table branch September 28, 2020 20:01
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.

3 participants