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

refactor(ui): simplify retry node by parametrizing retry workflow panel #13814

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Oct 25, 2024

Fixes #13343 (review)

Motivation

Simplifies and consolidates UI manual retry code -- 100+ LoC consolidated

Modifications

  • parametrize based on the optional props.nodeId

    • slightly different heading and no field selector input, but otherwise the same
    • props.onRetrySuccess can actually be used in both variants
    • also ensure tooltip is in both
    • and improve wording a bit
  • use the same retry panel element for both as (not just component)

    • which has nicer styling and animation, unlike the panel within a panel that was previously used
      • Screenshots:

        before:
        Screenshot 2024-10-24 at 9 02 25 PM

        after:
        Screenshot 2024-10-24 at 9 23 17 PM

  • fix types to not be optional in workflow-node-info.tsx (fixes feat(ui): Retry a single workflow step manually #13343 (comment)) and simplify code as such

  • plus a few small other simplifications as mentioned in my above linked review

Verification

Manually tested, works same as before, but with nicer styling + animation. See screenshots above

@agilgur5 agilgur5 added area/ui area/retry-manual Manual workflow "Retry" Action (API/CLI/UI). See retryStrategy for template-level retries labels Oct 25, 2024
@agilgur5 agilgur5 marked this pull request as ready for review October 25, 2024 01:26
@agilgur5
Copy link
Contributor Author

agilgur5 commented Oct 25, 2024

I couldn't request a review from @Adrien-D due to not being in the "members" team -- same reason as #8790 (comment) -- so tagging in this comment instead

EDIT: now possible after #13813 (comment)

@agilgur5 agilgur5 added this to the v3.6.0 milestone Oct 25, 2024
@agilgur5 agilgur5 force-pushed the refactor-ui-simplify-retry-node branch from 5d8294b to 690ad5a Compare October 25, 2024 17:17
- parametrize based on the optional `props.nodeId`
  - slightly different heading and no field selector input, but otherwise the same
  - `props.onRetrySuccess` can actually be used in both variants
  - also ensure tooltip is in both
  - and improve wording a bit

- disambiguate variable as `showRetryNodePanel` (missing "Panel" before)

- fix types to not be optional in `workflow-node-info.tsx` and simplify code as such

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
- one less piece of state
- nicer styling and nicer animation
- less code too

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 force-pushed the refactor-ui-simplify-retry-node branch from 61bb038 to 4fbe16a Compare October 28, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/retry-manual Manual workflow "Retry" Action (API/CLI/UI). See retryStrategy for template-level retries area/ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant