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

The suggest prompt with a required flag not working as expected #21

Closed
FrancisMawn opened this issue Aug 1, 2023 · 6 comments · Fixed by #25
Closed

The suggest prompt with a required flag not working as expected #21

FrancisMawn opened this issue Aug 1, 2023 · 6 comments · Fixed by #25
Assignees
Labels

Comments

@FrancisMawn
Copy link

Laravel Prompts Version

0.1.1

Laravel Version

10.14.1

PHP Version

8.1.2

Operating System & Version

Ubuntu 22.04.2 LTS

Terminal Application

VScode

Description

Hi,

It seems like a weird interaction between the "suggest" prompt and the required flag where the first time you select an option you get the "Required" error.

Steps To Reproduce

$database = suggest(
    label: 'Name of the database to dump',
    options: $this->databases(),
    required: true
);
  1. Press the down arrow key to choose a database from the list.
  2. Press ENTER to select the database
  3. Get the "Required" error message
  4. Pressing ENTER again clears the error and make the actual selection.
@devajmeireles
Copy link
Contributor

I tested, same here. 👍

@oreillysean
Copy link
Contributor

For me this is the fix

Index: src/SuggestPrompt.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/SuggestPrompt.php b/src/SuggestPrompt.php
--- a/src/SuggestPrompt.php	(revision ee79c23eff126556d0ec97b74e830884d61ef370)
+++ b/src/SuggestPrompt.php	(date 1690926555079)
@@ -45,7 +45,7 @@
     ) {
         $this->options = $options instanceof Collection ? $options->all() : $options;
 
-        $this->trackTypedValue($default);
+        $this->trackTypedValue($default, submit: false);
 
         $this->on('key', fn ($key) => match ($key) {
             Key::UP, Key::SHIFT_TAB => $this->highlightPrevious(),
@@ -133,10 +133,13 @@
     protected function selectHighlighted(): void
     {
         if ($this->highlighted === null) {
+            $this->submit();
             return;
         }
 
         $this->typedValue = $this->matches()[$this->highlighted];
+
+        $this->submit();
     }
 
     /**

but I do not have time to write a test. Maybe it might help.

Problem is the TypedValue.php trait is submitting before the selectHighlighted has run in SuggestPrompt

            foreach (str_split($key) as $key) {
                if ($key === Key::ENTER && $submit) {
                    $this->submit();

@crynobone
Copy link
Member

Can everyone retest this on Laravel Framework 10.17

@jessarcher jessarcher added the bug label Aug 2, 2023
@jessarcher jessarcher self-assigned this Aug 2, 2023
@jessarcher
Copy link
Member

Thanks for reporting this. I've confirmed the issue.

This seems to solve it for me, as it makes the SuggestPrompt event listeners execute before those from TypedValue.

-         $this->trackTypedValue($default);

          $this->on('key', fn ($key) => match ($key) {
              Key::UP, Key::SHIFT_TAB => $this->highlightPrevious(),
              Key::DOWN, Key::TAB => $this->highlightNext(),
              Key::ENTER => $this->selectHighlighted(),
              Key::LEFT, Key::RIGHT => $this->highlighted = null,
              default => (function () {
                  $this->highlighted = null;
                  $this->matches = null;
              })(),
          });

+         $this->trackTypedValue($default);

Can you please confirm whether this change would fix things for you? I can't see any regressions it would cause.

@jessarcher
Copy link
Member

I've created #25 to solve this. Please let me know if you discover any issues with it though.

Thanks again!

@Plytas
Copy link

Plytas commented Aug 2, 2023

Tested #25 and couldn't find any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants