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

Allow deleting steps using the keyboard #1017

Merged
merged 1 commit into from Jun 6, 2022

Conversation

BrunoOrchest
Copy link
Contributor

@BrunoOrchest BrunoOrchest commented Jun 3, 2022

Description

When user selects a step, the Title field is no longer auto focused to allow the delete/backspace keys to delete the step. In the case the step was just added, the Title field is focused so the user can start writing normally

Fixes: #556

Checklist

  • The documentation reflects the changes.
  • The PR branch is set up to merge into dev instead of master.
  • In case I changed one of the services’ models.py I have performed the appropriate database
    migrations (refer to scripts/migration_manager.sh).
  • In case I changed code in the orchest-sdk I followed its release
    checklist
  • In case I changed code in the orchest-cli I followed its release
    checklist
  • I haven't introduced breaking changes that would disrupt existing jobs, i.e. backwards
    compatibility is maintained.
  • In case I changed the dependencies in any requirements.in I have run pip-compile to update
    the corresponding requirements.txt.

Depending on wether a new step was added or just selected, the `Title` field will be focused or not allowing the user to press `Delete` to remove the step using the keyboard
Copy link
Contributor

@yannickperrenet yannickperrenet left a comment

Choose a reason for hiding this comment

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

Works like a charm! 👍

I quite like this behavior, feels "natural".

@iannbing iannbing self-requested a review June 3, 2022 10:28
@iannbing
Copy link
Contributor

iannbing commented Jun 3, 2022

Currently it seems that we have to carefully set the value of shouldAutoFocus back to false case by case. Is there a simpler way, for example, always reset shouldAutoFocus to false for all actions except forCREATE_STEP or DUPLICATE_STEPS?

@yannickperrenet yannickperrenet added the improvement An improvement or enhancement to an existing feature. label Jun 6, 2022
@BrunoOrchest
Copy link
Contributor Author

Currently it seems that we have to carefully set the value of shouldAutoFocus back to false case by case. Is there a simpler way, for example, always reset shouldAutoFocus to false for all actions except forCREATE_STEP or DUPLICATE_STEPS?

Not necessary since the SELECT_STEPS and related will take care of that. Other actions don't trigger the properties visibility so no need to be 'spamming' the code all over since the selection/deselection will take care of it always.

@BrunoOrchest BrunoOrchest merged commit 8441152 into dev Jun 6, 2022
@BrunoOrchest BrunoOrchest deleted the feat/delete-step-when-selected branch June 6, 2022 09:43
@iannbing
Copy link
Contributor

iannbing commented Jun 6, 2022

@BrunoOrchest You could set shouldAutoFocus as false before the switch instead of "spamming" in each case.

const state  = { ...currentState, shouldAutoFocus: false };

switch (action.type) {

     case "CREATE_STEP": {
          return {
                ...
                shouldAutoFocus: true,
           }
     } 

     // No need to set shouldAutoFocus to false in any other case.
}


The current implementation could lead to missing cases. For example:

  • there's an existing step A
  • create a new step
  • Immediately select step A: the title field is auto-focused. (because in case SET_OPENED_STEP, shouldAutoFocus is not set to false. Was it intended?).

@yannickperrenet yannickperrenet mentioned this pull request Sep 14, 2022
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement An improvement or enhancement to an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants