Skip to content

Conversation

@MatsJohansen87
Copy link
Collaborator

Description

Add input fields to Search bar

Related Issue

#498


How Has This Been Tested?

UI user test

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the DatePickerComponent should be less concerned with the search bar functionality.

To keep up/down arrow navigation functionality I think it would be fine to use a capture phase event handler (https://svelte.dev/tutorial/svelte/capturing) in the search bar drop down to intercept up and down arrows before they even reach the text input using event.stopPropagation(). For numbers this would be fine in my opinion. I don't consider the up/down arrow to increment/decrement numbers important. I hope the date picker calendar arrow key navigation would not break.

I think you could use a similar strategy for detecting focus without touching the DatePickerComponent. Or alternatively just add a onfocus prop to the DatePickerComponent and handle everything else outside of the component.


export default defineConfig([
globalIgnores(["dist", "book/book", "docs/assets"]),
globalIgnores(["dist", "book", "docs/assets"]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally I would want to lint the book markdown files. Can we fix the issues without disabling linting of those files completely?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. but book/book doesn't exist anywhere, so this was unnecessary, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

When you install mdbook and build the book locally it generates the HTML files there.

Copy link
Collaborator

@timakro timakro left a comment

Choose a reason for hiding this comment

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

The code in the search bar component looks much better now. And great that you added regex escaping to the search highlighting.

Do you think we can move some of the search bar and focus handling out of the input components? Maybe you can try my suggestion to intercept the arrow keys before the input components receive them. But I haven't thought a lot about that so I understand if you decide to stick with the current approach where you press escape to back out of the input component and then use the arrow keys.

Comment on lines +340 to +351
let inside: boolean = $state(false);
function handleClickInside(): void {
inside = true;
}
function handleClickOutside(): void {
if (!inside) {
autoCompleteOpen = false;
}
inside = false;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these click handlers needed? I tried without them and didn't noticed a difference

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should have handled the click onto an input element of the options but didn't work because the inside check was missing in handleFocusOut(). Added it and now it works as intended: when the click is outside the form elements but still inside of the input element it won't close.

}
let searchBarContainer: HTMLElement;
let optionsList: HTMLUListElement;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unused

Comment on lines +623 to +627
onmousedown={() =>
addInputValueToStore(
inputOption,
extractTargetGroupFromInputValue(),
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this do anything? The NumberInputComponent adds the item to the query itself.

@MatsJohansen87
Copy link
Collaborator Author

Maybe you can try my suggestion to intercept the arrow keys before the input components receive them. But I haven't thought a lot about that so I understand if you decide to stick with the current approach where you press escape to back out of the input component and then use the arrow keys.

I'm always careful when it comes to altering browser native behaviour. the user will expect the form fields to work the same way throughout the application. if we make that change, the input will behave differently from the ones in the catalogue. also it's a behaviour known to users for as long as they use the internet 😅

But maybe the convenience for the user is high enough to make that change. I guess this would come down to a short user test. I will recruit some non-IT-people.

@MatsJohansen87
Copy link
Collaborator Author

Do you think we can move some of the search bar and focus handling out of the input components?

I tried from the beginning and failed miserably 😄. 100% open to elegant solutions.

@timakro
Copy link
Collaborator

timakro commented Oct 6, 2025

I'm always careful when it comes to altering browser native behaviour.

I agree. In this case I think it might be worth it to prioritize the arrow keys but I'm not dead set on it.

100% open to elegant solutions

I'd try to use a capture phase event handlers (https://svelte.dev/tutorial/svelte/capturing) on the input components to catch the focusin and keydown events. If this works you could have the event handling code in the search bar component and reuse the handler functions between the different input component types. This should also get rid of some callback props if it works.

@patrickskowronekdkfz
Copy link
Collaborator

@MatsJohansen87 could you check why this feature is not working on safari? On chrome it works fine

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.

4 participants