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

Add BackButton #692

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Add BackButton #692

wants to merge 8 commits into from

Conversation

leolost2605
Copy link
Member

@leolost2605 leolost2605 commented Mar 1, 2024

Introduce a BackButton that automatically lables itself correctly with the preceding page and navigates back when clicked. Useful for elementary/switchboard#300 as well as in videos, appcenter, etc.

This will introduce an Adw dependency with minimum required version 1.4 which will break building granite on OS 7

@leolost2605 leolost2605 requested review from a team and danirabbit March 1, 2024 22:45
danirabbit and others added 3 commits March 2, 2024 01:10
@leolost2605
Copy link
Member Author

leolost2605 commented Mar 2, 2024

It now won't hide when there is no preceding page since that caused problems with it not being mapped again because it's not visible so that it won't recheck whether anything changed. It could still be implemented if needed but since there's usually always exactly one page that's the first and we know which one it is (Home on appcenter, category on settings, welcome on videos) I don't think it's worth the effort.

var previous_page = navigation_view.get_previous_page (navigation_page);

if (previous_page != null) {
label = previous_page.title;
Copy link
Contributor

@alice-mkh alice-mkh Mar 2, 2024

Choose a reason for hiding this comment

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

Note that page title can and will change.

The logic in libadwaita's own back buttons is also a lot more complicated because navigation views can be nested, and can also be structured like:

navigation split view
sidebar: navigation view
content: navigation view

and that situation is handled correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, tho the latter involves private api - forgot about that

Copy link
Member Author

@leolost2605 leolost2605 Mar 2, 2024

Choose a reason for hiding this comment

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

Note that page title can and will change.

Should be fixed although I think that would be kinda questionable UX while the page itself is not visible?

The logic in libadwaita's own back buttons is also a lot more complicated because navigation views can be nested, and can also be structured like:

navigation split view
sidebar: navigation view
content: navigation view

and that situation is handled correctly

I'm guessing that if the button is on the deepest view and there is no page left it still appears and instead navigates one above?
I think for now and for our use case it might be enough to leave that out (since we also don't hide when there is no preceding page)

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be fixed although I think that would be kinda questionable UX while the page itself is not visible?

Questionable yes, but I mean the property is writable, so...

I'm guessing that if the button is on the deepest view and there is no page left it still appears and instead navigates one above?

Correct.

I think for now and for our use case it might be enough to leave that out (since we also don't hide when there is no preceding page)

As you prefer

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding the nested part cc @danirabbit ?

@leolost2605 leolost2605 requested a review from alice-mkh March 2, 2024 13:36
@danirabbit
Copy link
Member

I think @tintou has some thoughts about adding Adw as a dep here in Granite

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