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

[VDG] Refactor navbar #9725

Merged
merged 6 commits into from Jan 9, 2023
Merged

[VDG] Refactor navbar #9725

merged 6 commits into from Jan 9, 2023

Conversation

soosr
Copy link
Collaborator

@soosr soosr commented Dec 9, 2022

Based on #9724, draft until that one gets merged.
Fixes: #8249

The main problem with the NavBar was that it was a ListBox and we relied on its SelectedItem property. That property was the trigger of the navigation.
This logic got problematic at a point as there are items in the NavBar that cannot be selected, so the idea is to have Buttons instead of the ListBoxItem in the NavBar, and the trigger of the navigation is the click that executes a command.

This PR:

  • Removes many negative margins and unnecessary code from the AXAML.
  • Removes the workarounds
  • Simplifies a lot

Known issue:

  • Navigation to another wallet B as soon as wallet A started to load breaks the navigation, the following PR will fix (including other fixes)

@soosr soosr marked this pull request as ready for review December 14, 2022 11:02
@soosr
Copy link
Collaborator Author

soosr commented Dec 14, 2022

Don't merge it before release.

Copy link
Collaborator

@nopara74 nopara74 left a comment

Choose a reason for hiding this comment

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

LGTM, id like @wieslawsoltes also to take a look

@wieslawsoltes
Copy link
Collaborator

LGTM

@soosr We can remove NavBarListBox as it's not used anymore.

@soosr soosr merged commit 2b5c761 into zkSNACKs:master Jan 9, 2023
@wieslawsoltes
Copy link
Collaborator

LGTM

@soosr We can remove NavBarListBox as it's not used anymore.

#9905

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings - blinking
3 participants