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

NavigationBar2 control #358

Merged
merged 8 commits into from
Nov 10, 2023
Merged

NavigationBar2 control #358

merged 8 commits into from
Nov 10, 2023

Conversation

promag
Copy link
Contributor

@promag promag commented Aug 27, 2023

This is an alternative to the existing NavigationBar component. The layout is such that the center section is horizontally centered when possible, and the right section is right aligned. Also, Loaders are avoided with this new approach.

Since the approach is quite different from the other navigation bar, a full refactor would be substantial, harder to test and subject to breaking changes. For that reason, both implementations will be available for a short period of time. More pull requests will follow to replace the old implementation once this is accepted.

Finally, the new navigation bar is used on the Peers, NodeRunner, and ThemeSettings pages.

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

tested ACK, desktop ver built on Ubuntu 22.04.

I've build #343 on top of this and used NavBar2 for the popup and the title was centered more accurately.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

Concept ACK

@jarolrod
Copy link
Member

jarolrod commented Sep 8, 2023

a full refactor would be substantial, harder to test and subject to breaking changes. For that reason, both implementations will be available for a short period of time. More pull requests will follow to replace the old implementation once this is accepted.

In that case, number the PR's and add a title for the series: (1/n) Reimplement NavigationBar Control

src/qml/controls/NavigationBar2.qml Outdated Show resolved Hide resolved
}
}

component Div: Pane {
Copy link
Member

Choose a reason for hiding this comment

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

nice, we should do this more often. Having helper components.


header: NavigationBar {
id: navbar
header: NavigationBar2 {
Copy link
Member

Choose a reason for hiding this comment

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

+1 for navbar2, this can be defined when defining the page, instead of defining the navbar components where the Peers page is to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what is the suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

There is no suggestion, just a comment on what I like about this :)

src/qml/controls/NavigationBar2.qml Show resolved Hide resolved
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

tACK b5b73b0

This is an improvement over using loaders in the original NavigationBar, and sets a foundation for where the navbar needs to go. Last place that still uses the original navbar is InformationPage, but this requires more reworking and the work needed can change depending on when we move to a UI flow of stack views.

@hebasto hebasto merged commit 46e93de into bitcoin-core:main Nov 10, 2023
6 checks passed
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.

4 participants