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

Pre-select autocomplete file when passing file path #19108

Merged

Conversation

MahmoudHamdy02
Copy link
Contributor

No description provided.

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Would be super nice to add some test

check testReactPatterns in test/verify/check-pages

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

So although it generally works i think you overcomplicated this a bit. type can be autodetected from the component code. I suggest the following code (applied on top of your current branch) - its in my opinion a bit cleaner. Let me know what you think:

diff --git pkg/lib/cockpit-components-file-autocomplete.jsx pkg/lib/cockpit-components-file-autocomplete.jsx
index 378ff183e..8b9e8f546 100644
--- pkg/lib/cockpit-components-file-autocomplete.jsx
+++ pkg/lib/cockpit-components-file-autocomplete.jsx
@@ -32,13 +32,9 @@ export class FileAutoComplete extends React.Component {
             directory: '', // The current directory we list files/dirs from
             displayFiles: [],
             isOpen: false,
             value: this.props.value || null,
             type: this.props.type
         };
-        if (this.props.type === "file") {
-            this.props.onChange(this.props.value.slice(this.props.value.lastIndexOf("/") + 1));
-        }
-        this.updateFiles(props.value ? this.props.type === "file" ? props.value.slice(0, props.value.lastIndexOf("/") + 1) : props.value : "/");
         this.typeaheadInputValue = "";
         this.allowFilesUpdate = true;
         this.updateFiles = this.updateFiles.bind(this);
@@ -47,7 +43,7 @@ export class FileAutoComplete extends React.Component {
         this.clearSelection = this.clearSelection.bind(this);
         this.onCreateOption = this.onCreateOption.bind(this);
 
-        this.debouncedChange = debounce(300, (value) => {
+        this.onPathChange = (value) => {
             if (!value) {
                 this.clearSelection();
                 return;
@@ -84,7 +80,9 @@ export class FileAutoComplete extends React.Component {
                     return this.updateFiles(parentDir + '/');
                 }
             }
-        });
+        };
+        this.debouncedChange = debounce(300, this.onPathChange);
+        this.onPathChange(this.state.value);
     }
 

@MahmoudHamdy02
Copy link
Contributor Author

@KKoukiou definitely cleaner! I've pushed the suggested code

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

This is good now, but can you add a test for it as suggested here #19108 (review)

test/verify/check-pages Outdated Show resolved Hide resolved
@MahmoudHamdy02 MahmoudHamdy02 changed the title Add type prop to autocomplete component to pass file path Pre-select autocomplete file when passing file path Jul 28, 2023
test/verify/check-pages Outdated Show resolved Hide resolved
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

And please squash all commits with next push.

@MahmoudHamdy02 MahmoudHamdy02 marked this pull request as ready for review July 31, 2023 10:28
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Hm tests now fail with mkdir: cannot create directory ‘/home/admin/newdir’: File exists

I was pretty sure that /home/admin directory get's cleanup but aparently no. Sorry for wrong hint. Please put self.restore_dir('/home/admin') in the start of the test.

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Needs squash on merge.

@KKoukiou KKoukiou merged commit 848bccc into cockpit-project:main Aug 2, 2023
30 checks passed
@MahmoudHamdy02 MahmoudHamdy02 deleted the pass-file-to-autocomplete branch August 2, 2023 11: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.

2 participants