Skip to content

Conversation

@d2dyno1
Copy link
Member

@d2dyno1 d2dyno1 commented Jul 25, 2024

Resolved / Related Issues

Changes

This PR introduces highly requested forward and backward navigation history feature. You can access the list by right-clicking either of the toolbar buttons to view a list of recently navigated folders. Selecting any entry will bring you to that folder

To Do

  • Code clean up (sort and move around classes/methods)
  • Implement button holding for touch devices
  • Add icon for HomePage

Screenshots

image

@d2dyno1 d2dyno1 requested a review from yaira2 July 25, 2024 21:44
@yaira2
Copy link
Member

yaira2 commented Jul 25, 2024

Is TeachingTip preferred over MenuFlyout?

@d2dyno1
Copy link
Member Author

d2dyno1 commented Jul 25, 2024

Is TeachingTip preferred over MenuFlyout?

It's better because it allows a ListView to be utilized with ItemsSource binding, whereas a MenuFlyout doesn't easily bind to an ObservableCollection of items. Moreover, when button holding is added, MenuFlyout may not work as expected.

I personally think a TeachingTip looks better since it always opens in the same place MenuFlyout on the other hand would open at the cursor's position

@yaira2
Copy link
Member

yaira2 commented Jul 26, 2024

I'm not convinced it's the best UX to use here, but it's alright for now.

@yaira2 yaira2 marked this pull request as ready for review July 28, 2024 15:03
@yaira2 yaira2 changed the title Added navigation history to AddressToolbar Feature: Added navigation history flyout when right clicking the back/forward buttons Jul 28, 2024
@yaira2 yaira2 added the ready for review Pull requests that are ready for review label Jul 28, 2024
@yaira2
Copy link
Member

yaira2 commented Jul 28, 2024

@d2dyno1 I found an issue where Local Disk doesn't display the label.
image

@0x5bfa
Copy link
Member

0x5bfa commented Jul 28, 2024

a MenuFlyout doesn't easily bind to an ObservableCollection of items.

Too bad that they didn't make any progress over a half decade https://github.com/microsoft/microsoft-ui-xaml/issues/1087 but IMO we can create a attached property 'ItemsSource' and put it via MenuFlyout.Items.Add().

MenuFlyout on the other hand would open at the cursor's position

If it's set to Button.Flyout it should be at the exact correct position.

@yaira2
Copy link
Member

yaira2 commented Jul 28, 2024

We can copy what we do for the breadcrumb flyouts

yaira2
yaira2 previously approved these changes Aug 4, 2024
Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

Behavior looks good. Can you resolve the merge conflicts?

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed ready for review Pull requests that are ready for review labels Aug 4, 2024
Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

Looks good to me, code wise.

@yaira2 yaira2 merged commit 9ae6397 into files-community:main Aug 6, 2024
@yaira2 yaira2 changed the title Feature: Added navigation history flyout when right clicking the back/forward buttons Feature: Added navigation history flyout to the back/forward buttons Aug 6, 2024
@d2dyno1 d2dyno1 deleted the d2/f_history branch August 6, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready to merge Pull requests that are approved and ready to merge

3 participants