Skip to content

Feature: Introduced Omnibar 1 #17023

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Apr 6, 2025

Resolved / Related Issues

Steps used to test these changes

  1. Open Files app
  2. Enable Omnibar from Settings > Advanced
  3. Use it

image

@0x5bfa 0x5bfa force-pushed the 5bfa/CQ-OmnibarIntroduction branch 4 times, most recently from f93960a to e1ee482 Compare April 10, 2025 02:54
Copy link
Member

Choose a reason for hiding this comment

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

It looks like there are some extra modifications to this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good.

if (e.IsRootItem)
{
// TODO: Populate a different flyout for the root item
e.Flyout.Items.Add(new MenuFlyoutHeaderItem() { Text = "Quick access" });
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the placeholder menu? I would prefer not merging placeholder code.

@@ -198,17 +206,56 @@ public bool IsSearchBoxVisible
}
}

private string? _PathText;
public string? PathText
private string? _LegacySharedPathPaletteText;
Copy link
Member

Choose a reason for hiding this comment

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

Unless the property name is being reused, I would feel more comfortable if we kept the existing property names. Otherwise it will require additonal testing and a more thorough review.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -417,20 +417,20 @@ protected async void ShellPage_PathBoxItemDropped(object sender, PathBoxItemDrop

protected async void ShellPage_AddressBarTextEntered(object sender, AddressBarTextEnteredEventArgs e)
{
await ToolbarViewModel.SetAddressBarSuggestionsAsync(e.AddressBarTextField, this);
await ToolbarViewModel.SetLegacyAddressBarSuggestionsAsync(e.AddressBarTextField, this);
Copy link
Member

Choose a reason for hiding this comment

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

Same as above ^

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@yaira2
Copy link
Member

yaira2 commented Apr 21, 2025

I found an issue where text entered into the address bar isn't cleared after removing the focus. The easiest way to test this is by removing the focus and then refocusing the address bar.

@0x5bfa 0x5bfa changed the title Feature: Introduced Omnibar Feature: Introduced Omnibar 1 Apr 22, 2025
@0x5bfa 0x5bfa force-pushed the 5bfa/CQ-OmnibarIntroduction branch from 8000bab to 0aeb079 Compare April 22, 2025 09:14
@0x5bfa 0x5bfa force-pushed the 5bfa/CQ-OmnibarIntroduction branch from 0aeb079 to e4ba94d Compare April 22, 2025 09:16
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