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

fix(web): broken search-bar during page load #3548

Merged
merged 2 commits into from
Aug 5, 2023

Conversation

martabal
Copy link
Member

@martabal martabal commented Aug 4, 2023

This PR fixes :

  • The search bar not properly displayed during the page load.
  • Save in the search history m: too. Before, if the user searched m:word, only word was saved
Before After
2023-08-04.13-48-30.mp4
2023-08-04.13-47-29.mp4

@vercel
Copy link

vercel bot commented Aug 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
immich ⬜️ Ignored (Inspect) Visit Preview Aug 4, 2023 4:52pm

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Why does this work? Would changing the original button to display block also work?

@martabal
Copy link
Member Author

martabal commented Aug 4, 2023

I tried it and it does not work. Somehow the button is not rendered until the page is fully loaded.

@alextran1502
Copy link
Contributor

Does this change affect the action of clicking on pass search result or to remove them from the list?

@martabal
Copy link
Member Author

martabal commented Aug 4, 2023

@alextran1502 Tested and it didn't change.
I noticed a strange behavior on the search page. When you press 'Escape' with the search bar focused, it unfocus it AND go back to the previous page at the same time. It should unfocus the search bar if it's focused and go back to the previous page if it's not. But I can't use isSearchEnabled because the outside click is executed before the key handler on the search page.

Before After
2023-08-04.18-55-11.mp4
2023-08-04.18-55-33.mp4

@jrasm91
Copy link
Contributor

jrasm91 commented Aug 4, 2023

I've seen quite a few issues related to keyboard shortcuts and it seems to be getting pretty complicated, so I thought I'd just share a few thoughts.

  • DOM events propogate up to parent elements until you call stop propogation.
  • Some keys have default browser actions, like the arrow keys and certain keyboard shortcuts like CTRL + F = find, space bar outside of a text field scrolls, etc. In general we shouldn't conflict/prevent these, but the default action will happen unless you call prevent default
  • We should be careful when we call stop propogation and prevent default as I think this is where a lot of our bugs come from. We should only do that when we handle an event and don't want it to cause other actions to occur.
  • In general we don't want to prevent default behavior when the user in typing in a text field or has selected text.

@alextran1502 alextran1502 merged commit f1b9271 into main Aug 5, 2023
21 checks passed
@alextran1502 alextran1502 deleted the fix/broken-search-bar branch August 5, 2023 18:05
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.

None yet

3 participants