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

fix aggregation sorting in browsev2 sidebar #8276

Conversation

joshuaeilers
Copy link
Contributor

@joshuaeilers joshuaeilers commented Jun 21, 2023

Changes

  • Sort aggregations by display name on the frontend
  • Align counts vertically
image

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@joshuaeilers joshuaeilers marked this pull request as ready for review June 21, 2023 16:18
@joshuaeilers joshuaeilers force-pushed the je--fix-aggregation-sorting-v2 branch from d4e8488 to 9b10d2a Compare June 21, 2023 16:18
@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Jun 21, 2023
Comment on lines 65 to 67
const nameA = registry.getDisplayName(EntityType.DataPlatform, a.entity);
const nameB = registry.getDisplayName(EntityType.DataPlatform, b.entity);
return nameA.localeCompare(nameB);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Safe to assume these will always have an entity? I suppose we could fallback to sorting by the agg value if the entity wasn't present for some reason.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do think falling back to the agg value if the entity isn't present is safer here.. sometimes we see data get ingested on a platform where the platform entity has not been ingested (this is a bad state, but it does happen)

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

just checking if the entity of a platform agg exists first is a safe bet I think!

good call out - I think I need to make a quick tweak to filters in case the platform entity isn't there.

to be clear i think this is very unlikely, but still would be nice to guard against

Comment on lines 65 to 67
const nameA = registry.getDisplayName(EntityType.DataPlatform, a.entity);
const nameB = registry.getDisplayName(EntityType.DataPlatform, b.entity);
return nameA.localeCompare(nameB);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do think falling back to the agg value if the entity isn't present is safer here.. sometimes we see data get ingested on a platform where the platform entity has not been ingested (this is a bad state, but it does happen)

@joshuaeilers
Copy link
Contributor Author

just checking if the entity of a platform agg exists first is a safe bet I think!

good call out - I think I need to make a quick tweak to filters in case the platform entity isn't there.

to be clear i think this is very unlikely, but still would be nice to guard against

Didn't address all filters but did make a fix to getFilterIconAndLabel. The prob here is that our EntityRegistry on the FE allows null values through without complaint from Typescript. On my backlog to fix that, but it's a little tricky.

Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

beautiful

@joshuaeilers joshuaeilers enabled auto-merge (squash) June 21, 2023 20:03
@joshuaeilers joshuaeilers merged commit 42260fc into datahub-project:master Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants