-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
App navigation via unified search #22102
App navigation via unified search #22102
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2cb8704
to
7aa61af
Compare
0af7698
to
71b62c4
Compare
0080616
to
507f395
Compare
Rebased with order adjustments |
Signed-off-by: Joas Schilling <coding@schilljs.com>
507f395
to
fcdd702
Compare
$result[] = new SearchResultEntry( | ||
'', | ||
$entry['name'], | ||
'', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, is there no other way for an API to have empty strings like that? Is that good practices?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 👍 to @skjnldsv’s feedback, and also I found one entry which doesn’t really make sense – there’s possibly more which we might need to filter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feedback can also be fixed in a follow-up tho.
…tatus) Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Well user status is registering a navigation link which is highjacked by JS. I'm not aware we have more of this, but now fixed by ignoring empty link items Fix both issues. |
🤖 beep boop beep 🤖 Here are the logs for the failed build: Status of 31430: failureacceptance-app-files
Show full log
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How nice is this?!? :) I love it already 👍
Branch targets #22099