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

Feature: FlowList and DeploymentList components #1346

Merged
merged 122 commits into from
Apr 19, 2023

Conversation

znicholasbrown
Copy link
Contributor

This PR:

New:

  • FlowList component; functionally equivalent to the FlowsTable
  • FlowListItem component; functionally equivalent to a row in the FlowTable except contains a list of its deployments, used in FlowList
  • DeploymentList component; functionally equivalent to the DeploymentsTable, used in FlowListItem
  • DeploymentListItem component, functionally equivalent to a row in the DeploymentsTable, used in FlowListItemDeployments
  • lastFlowRun composition; creates a subscription for the most recent flow run (by expected start time) matching the passed filter and returns a ComputedRef for the run and the subscription itself
  • nextFlowRun composition; creates a subscription for the next flow run (by expected start time) matching the passed filter and returns a ComputedRef for the run and the subscription itself

Enhanced:

  • deployment, flow, and flow run mock APIs all support basic filtering and sorting
  • StateListItem component now supports an action slot, along with several other enhancements (non-breaking, should all be purely additive)

Fixed:

  • mock data store utilities are now properly hydrating date objects for flows, deployments, and flow runs and schedule objects from deployments

@znicholasbrown
Copy link
Contributor Author

@brandonreid fixed the border radius issue; I'm not against an icon per-se but I also don't want to shoe-horn them into this design when we're not using them for top-level items anywhere else.

@znicholasbrown
Copy link
Contributor Author

Also this PR is missing commits atm cause of the ongoing github issues - some of the fixes won't yet be applied

@znicholasbrown
Copy link
Contributor Author

Ok looks like all commits are showing up and should be in the demo now

Copy link
Collaborator

@pleek91 pleek91 left a comment

Choose a reason for hiding this comment

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

Sorry for so many comments. There's a lot here that's really good. And I love the direction. Just wanting to really iron this out since its a significant rewrite.

demo/compositions/useFlowMock.ts Show resolved Hide resolved
src/components/DeploymentsDeleteButton.vue Show resolved Hide resolved
src/components/FlowsDeleteButton.vue Show resolved Hide resolved
src/components/ListItemMeta.vue Outdated Show resolved Hide resolved
src/components/PageHeadingFlows.vue Show resolved Hide resolved
src/components/FlowListItem.vue Outdated Show resolved Hide resolved
src/components/FlowList.vue Outdated Show resolved Hide resolved
src/components/FlowList.vue Outdated Show resolved Hide resolved
src/components/FlowList.vue Outdated Show resolved Hide resolved
src/components/DeploymentListItem.vue Outdated Show resolved Hide resolved
@znicholasbrown
Copy link
Contributor Author

So I went ahead and wrote useDeployments, useDeploymentsCount, useFlowsCount compositions and modified the existing useFlows composition to add a new overload for passing flow ids or a flows filter; lmk what you think of that implementation. The one caveat is that I retained permissions checks in the filter the composition uses, which means that technically every component will still need to do objectList ?? [] because there's a chance a user is missing permissions and the subscription never runs, according to the UseEntitySubscription overload. We could relax that type a bit but I think it makes sense as a generic and the undefined return might be a useful indicator.

All other feedback has also been addressed 😄

Copy link
Collaborator

@pleek91 pleek91 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for working through all the details with me!

@znicholasbrown znicholasbrown merged commit ffc02d4 into main Apr 19, 2023
@znicholasbrown znicholasbrown deleted the feature-flow-deployment-component-2023-04-13 branch April 19, 2023 14:56
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.

4 participants