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.

Sign up for GitHub

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

Jump to bottom

feat: Allows for splitting stats' distinct values into a different element that shows in modal #960

Merged
merged 11 commits into from
Apr 1, 2021
Merged

feat: Allows for splitting stats' distinct values into a different element that shows in modal #960

merged 11 commits into from
Apr 1, 2021

Conversation

Golodhros
Copy link
Member

@Golodhros Golodhros commented Mar 29, 2021 •
edited
Loading

Summary of Changes

This is part of the work around table stats
Splits distinct values from the regular stats table
Consolidates stats utils in one file
Adds a configuration option to add the distinct values stat name to the configuration

Tests

Added tests for new components
Tested utils

Screenshots

coco_dim_driver_-_Amundsen_Table_Details

coco_dim_driver_-_Amundsen_Table_Details

Documentation

Soon

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

@Golodhros Golodhros force-pushed the mi-ds-unique-values branch from 6d52e68 to 84c09cc Compare March 29, 2021 20:13
@Golodhros Golodhros requested a review from a team as a code owner March 29, 2021 21:27
@Golodhros Golodhros force-pushed the mi-ds-unique-values branch from 4ef78e6 to 1f856c4 Compare March 29, 2021 21:40
@Golodhros Golodhros changed the title feat: Splits unique values into a different element and includes modal feat: Allows for splitting stats' distinct values into a different element that shows in modal Mar 29, 2021
@Golodhros Golodhros requested a review from danwom March 29, 2021 21:44
@allisonsuarez
Copy link
Contributor

Code looks good to me but referencing back to the design isn't the modal supposed to be less wide?

@allisonsuarez
Copy link
Contributor

👍

@Golodhros
Copy link
Member Author

Code looks good to me but referencing back to the design isn't the modal supposed to be less wide?

Yeah, I changed it already, that's an old screenshot.

allisonsuarez
allisonsuarez approved these changes Mar 30, 2021
View reviewed changes
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!

danwom
danwom reviewed Apr 1, 2021
View reviewed changes
Copy link

@danwom danwom left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good. Left some comments to help with readability.

amundsen_application/static/js/features/ColumnList/ColumnStats/index.tsx Outdated
@@ -41,8 +42,9 @@ const ColumnStats: React.FC<ColumnStatsProps> = ({
if (stats.length === 0) {
return null;
}
const startEpoch = Math.min(...stats.map(getStart));
const endEpoch = Math.max(...stats.map(getEnd));
const filteredStats = filterOutUniqueValues(stats);
Copy link

Choose a reason for hiding this comment

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

Just a nit about readability: We're using the both distinct (distinctStatTypeName) and unique (uniqueValues) somewhat interchangeably. I think it would be better to use either distinct or unique consistently for both the configs and util functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was using the unique values name until I found they call them distinct on the backend.

you are right, I will rename the configuration.

amundsen_application/static/js/features/ColumnList/index.tsx Outdated Show resolved Hide resolved
Marcos Iglesias added 11 commits March 31, 2021 20:51
Summary of values
b6af64c 
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Tested ExpandableUniqueValues and consolidated helpers related to stats
0ba6bb6 
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Plugging the modal, not tested on app yet
b58c0f4 
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Tweaking link and comma separated list
a71e002 
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Modal showing with right sizes and tweaking spacing on summary
2e012e9 
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Add tooltip to see all button
c76e75b 
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Moved hardcoded distinct values key into the configuration
2ce3806 
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Updatind docs
694b36c 
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Removing naming convention betterer checks
a52c7f7 
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Updating proptypes rule, simplifying stats logic
5199af1 
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Renaming distinct to unique values all over
86605b1 
Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
@Golodhros Golodhros force-pushed the mi-ds-unique-values branch from d5281cc to 86605b1 Compare April 1, 2021 03:52
@Golodhros Golodhros merged commit fe04a06 into master Apr 1, 2021
@Golodhros Golodhros deleted the mi-ds-unique-values branch April 1, 2021 17:07
Golodhros added a commit that referenced this pull request Apr 2, 2021
@Golodhros
feat: Allows for splitting stats' distinct values into a different el…
91ae2b3 
…ement that shows in modal (#960)

* Summary of values

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

* Tested ExpandableUniqueValues and consolidated helpers related to stats

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

* Plugging the modal, not tested on app yet

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

* Tweaking link and comma separated list

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

* Modal showing with right sizes and tweaking spacing on summary

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

* Add tooltip to see all button

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

* Moved hardcoded distinct values key into the configuration

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

* Updatind docs

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

* Removing naming convention betterer checks

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

* Updating proptypes rule, simplifying stats logic

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

* Renaming distinct to unique values all over

Signed-off-by: Marcos Iglesias <miglesiasvalle@lyft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwom danwom danwom left review comments

@allisonsuarez allisonsuarez allisonsuarez approved these changes

@dikshathakur3119 dikshathakur3119 Awaiting requested review from dikshathakur3119 dikshathakur3119 is a code owner

@dorianj dorianj Awaiting requested review from dorianj

@feng-tao feng-tao Awaiting requested review from feng-tao feng-tao is a code owner

@ttannis ttannis Awaiting requested review from ttannis ttannis is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Golodhros @allisonsuarez @danwom