Skip to content
This repository has been archived by the owner on Nov 13, 2024. It is now read-only.

dex 1117 - Replace progress with pipeline_status in Asset Group #420

Merged

Conversation

Emily-Ke
Copy link
Contributor

@Emily-Ke Emily-Ke commented Jul 7, 2022

Pull request checklist:

  • Features and bugfixes should be PRed into the develop branch, not main
  • All text is internationalized No user facing text
  • There are no linter errors
  • New features support all states (loading, error, etc)

Pull Request Overview

This is the expected behavior:

progress status pipeline status front end
progress doesn't exist complete no alert, table displays with asset counts, no data refetch
created inProgress in progress alert, table displays asset counts as "pending", poll API for data
healthy inProgress in progress alert, table displays asset counts as "pending", poll API for data
completed complete no alert, able displays with asset counts, no data refetch
skipped skipped no alert, table displays with asset counts, no refetch
cancelled waiting in progress alert, table displays asset counts as "pending", poll API for data
failed failed error alert, table displays asset counts as "pending", no refetch

@Emily-Ke Emily-Ke marked this pull request as ready for review July 7, 2022 21:42
@Emily-Ke Emily-Ke requested review from brmscheiner and Atticus29 July 7, 2022 21:42
Copy link
Contributor

@Atticus29 Atticus29 left a comment

Choose a reason for hiding this comment

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

LGTM

resultData?.data?.progress_preparation || {};
const pipelineStatus = get(
resultData,
'data.pipeline_status.preparation',
Copy link
Contributor

Choose a reason for hiding this comment

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

I love that we all prefer using get() in a slightly different way lol.

Copy link
Contributor

@brmscheiner brmscheiner left a comment

Choose a reason for hiding this comment

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

two small things lgtm overall

</>
)}
{!isPreparationFailed &&
!isProgressSettled(pipelineStatusPreparation) && (
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't love these nested &&s. possible to set a variable to isPreparationFailed && !isProgressSettled(pipelineStatusPreparation) above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I restored the showPreparationInProgressAlert variable in ec3b7a0

<>
<LinearProgress
style={{
borderTopLeftRadius: '4px',
Copy link
Contributor

Choose a reason for hiding this comment

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

could be 4 instead of '4px'

Copy link
Contributor

Choose a reason for hiding this comment

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

(there are a couple more of these further down in the file too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in ec3b7a0

@Emily-Ke Emily-Ke merged commit 4ec0222 into develop Jul 12, 2022
@Emily-Ke Emily-Ke deleted the dex-1117-asset-group-remove-dependence-on-progress-object branch July 12, 2022 21:13
Emily-Ke added a commit that referenced this pull request Jul 29, 2022
* Automatic Docker builds on tagged releases and PRs with multi-platform support

* Remove nightly action, which was merged into build

* Allow all files to be copied in

* Dex 761 social group site settings (#419)

* DEX-761 flesh out social group form except for multipleInGroup

* DEX-761 begin fleshing out the switch

* DEX-761 continue with fleshing out the switch

* DEX-761 add useEffect

* DEX-761 rename the role component, clean up, and remove the unnecessary useState

* DEX-761 internationalize and add description as caption

* DEX-761 respond to code review feedback

* DEX-761 remove unecessary flexbox div

* DEX-761 handle the case where a guidless role makes it into the SocialGroupRole component

* move server status components to siteStatus directory

* Change server status text to site status

* Add server status SuperText keys, prevent 'hide' prop from passing to Text

* Internationalize aria labels in site status components

* dex 1117 - Replace progress with pipeline_status in Asset Group (#420)

* Replace asset group progress with pipeline_status

* Change px to number and abstract showAlert in AssetGroup

* Add public AGS page (#416)

* Add public AGS page

* Remove unused property

* Fixes

* Rename things

* Remove unused translation key

* use new terms (#418)

* begin release procedure (#424)

* begin release procedure

* clarify about merge conflicts

* respond to feedback

* respond to code review feedback

* Update release procedure doc

* Run on sentence

Co-authored-by: Ben Scheiner <[email protected]>

* DEX-922 implement stickyHeader and add a meaningful maxHeight to ever… (#430)

* DEX-922 implement stickyHeader and add a meaningful maxHeight to every DataDisplay. Also add horizontalScroll

* DEX-922 remove seemingly unnecessary horizontal scroll prop

* DEX-922 respond to code review feedback. Remove default maxHeight for Card component and replaced with optional styles, which was then implemented for ImageCard and StatusCard. Remove maxHeight from Card in relationshipCard, remove unused paperStyles from DataDisplay.

* DEX-922 respond to code review feedback pt 2

* DEX-922 add tableContainerStyles to DataDisplay and remove maxHeight

* DEX-922 remove tableContainerStyles from SightingsDisplay and fix a few little things

* DEX-922 final little fixes

* Compute new form state from previous state in Settings (#433)

* 922 follow up (#435)

* DEX-922 print the collaboration data to make the stackBlitz question easier

* DEX-922 I do not understand why tableData was undefined, but cool, codex, I can play this game with you

* DEX-922 try disablePortal

* DEX-922 change the z-index

* DEX-922 clean up

* DEX-922 respond to code review feedback

* Update version to 1.0.2 (#436)

Co-authored-by: Jason Parham <[email protected]>
Co-authored-by: Mark <[email protected]>
Co-authored-by: Ben Scheiner <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants