Skip to content

Conversation

mabaasit
Copy link
Collaborator

@mabaasit mabaasit commented Apr 5, 2022

feat(aggregations): display aggregation results COMPASS-5669

  1. Resolves open issues in feat(aggregations): run aggregation COMPASS-5549 #2930 and created a follow up COMPASS-5681 for error state.
  2. Updated CancelLoader in compass components.
Screen.Recording.2022-04-06.at.17.06.04.mov

Description

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@mabaasit mabaasit marked this pull request as ready for review April 6, 2022 14:37
@mabaasit mabaasit requested a review from mcasimir April 7, 2022 13:37
Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

When in "Running" state, maybe it's worth not hiding pagination completely to avoid content flashing in and out all the time (we handle it similarly in the CRUD view), what do you think?

@mabaasit
Copy link
Collaborator Author

When in "Running" state, maybe it's worth not hiding pagination completely to avoid content flashing in and out all the time (we handle it similarly in the CRUD view), what do you think?

Good idea. Just added it.

@mabaasit mabaasit requested a review from gribnoysup April 11, 2022 15:16
</button>
</div>
<div className={containerStyles} data-testid={dataTestId}>
<SpinLoader size="24px" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

spacing[4]?

}
}

.loader {
Copy link
Collaborator

Choose a reason for hiding this comment

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

just curious, why was this necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice. the CancelLoader component previously centered the content and also added height: 100%. I removed that and made component occupy width/height it needed and let consumer of the component style it as needed.

flex-shrink: 1;
}

.loader {
Copy link
Collaborator

Choose a reason for hiding this comment

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

mostly the same question as above :)

@mcasimir
Copy link
Collaborator

🔥🔥🔥

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Seems like disabling stages still shows them in the pipeline preview:

image

I wonder if there is something we can do to make it more obvious that some stages are disabled. I'll add something for this to the design sync.

Another small thing is that scrollbar here is stuck to the items, overlapping with them:

image

whereas on the builder side it is at the side of the screen. Would be nice to align them, maybe as a follow-up PR.

Otherwise looks really good and works perfectly, great work!

@mabaasit mabaasit merged commit 3f4cfe9 into main Apr 14, 2022
@mabaasit mabaasit deleted the COMPASS-5669-aggregate-results branch April 14, 2022 14:15
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.

5 participants