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

Enable setting the backfill counter of all task bindings with a single action #965

Merged
merged 23 commits into from
Feb 8, 2024

Conversation

kiahna-tucker
Copy link
Member

@kiahna-tucker kiahna-tucker commented Feb 5, 2024

Issues

The issues directly below are completely resolved by this PR:
#962

The issues directly below are advanced by this PR:
#973

Changes

962

The following features are included in this PR:

  • Add a Backfill section directly above the bindings selector in edit workflows that contains a header, short description, and an outlined, toggle button with the label Backfill. This button should have the same styling as the binding-specific Backfill button.

  • Update the backfill counter of applicable, drafted bindings when the new backfill button is clicked. The toggle button state triggers are as follows:

    • Disabled: the button will be in a disabled state when an asynchronous task that updates the server is in progress and when the drafted specification does not contain at least one enabled binding.

    • Deselected: the button will be in a deselected state by default and when the backfill counter of any drafted binding has not been incremented in the editing session.

    • Selected: the button will be in a selected state when the backfill counter of all drafted bindings have been incremented in the editing session.

NOTE: The backfill counter of a drafted binding does not need to be reset if the binding is disabled post-incrementation in the editing session.

973

The following features are included in this PR:

  • Add disabled state styling to the OutlinedToggleButton.

Tests

Manually tested

  • scenarios you manually tested

Automated tests

N/A

Screenshots

Capture edit | Bindings enabled | Backfill unset

pr_screenshot-965-manual_backfill_all-capture-edit-unselected

Capture edit | Bindings enabled | Backfill set

pr_screenshot-965-manual_backfill_all-capture-edit-selected

Capture edit | Bindings disabled

pr_screenshot-965-manual_backfill_all-capture-edit-all_bindings_disabled

Materialization edit | Bindings enabled | Backfill unset

pr_screenshot-965-manual_backfill_all-materialization-edit-unselected

Materialization edit | Bindings enabled | Backfill set

pr_screenshot-965-manual_backfill_all-materialization-edit-selected

Materialization edit | Bindings disabled

pr_screenshot-965-manual_backfill_all-materialization-edit-all_bindings_disabled

Disabled primary colored outlined toggle button

pr_screenshot-965-manual_backfill_all-disabled_button_styling

@kiahna-tucker kiahna-tucker marked this pull request as ready for review February 6, 2024 22:14
@kiahna-tucker kiahna-tucker requested a review from a team as a code owner February 6, 2024 22:14
@@ -443,7 +446,7 @@ const getInitialState = (
},

prefillBackfilledCollections: (liveBindings, draftedBindings) => {
const { addBackfilledCollection } = get();
const { addBackfilledCollections } = get();
Copy link
Member

Choose a reason for hiding this comment

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

Why are we having this stand alone function that loops through all the bindings?

We are already calling prefillResourceConfig right before calling this which means we loop through all the bindings twice in a row.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you like to conflate the scope of prefillResourceConfig?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah - as best we can I would like that big initial looping to only be done once if possible.

Copy link
Member

@travjenkins travjenkins left a comment

Choose a reason for hiding this comment

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

Some call outs. All of `em do not need handled right now. I think the ones that need handled I marked with a ❤️

Copy link
Member

@travjenkins travjenkins left a comment

Choose a reason for hiding this comment

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

lgtm

tested a bit and seemed alright with 1k collections

@kiahna-tucker kiahna-tucker merged commit 263164a into main Feb 8, 2024
3 checks passed
@kiahna-tucker kiahna-tucker deleted the kiahna-tucker/backfill/add-mass-action branch February 8, 2024 17:12
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.

2 participants