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 YaruMasterDetailPage.orientationOf() and appBarBuilder #642

Merged
merged 5 commits into from
Mar 6, 2023
Merged

Add YaruMasterDetailPage.orientationOf() and appBarBuilder #642

merged 5 commits into from
Mar 6, 2023

Conversation

jpnurmi
Copy link
Member

@jpnurmi jpnurmi commented Feb 26, 2023

No description provided.

Copy link
Member

@Jupi007 Jupi007 left a comment

Choose a reason for hiding this comment

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

Very cool, LGFM 👍

you just need to fix a missing comma

@jpnurmi
Copy link
Member Author

jpnurmi commented Feb 26, 2023

@Feichtmeier does this help with the back button issue? i have a feeling it doesn't because if Navigator.of(context).canPop() didn't work for you, it sounds like you're building the title bar outside the master-detail page so you can't query the MD orientation via an inherited widget like this either?

@Feichtmeier
Copy link
Member

ah you mean because

return Column(
      children: [
        appBar,
        Expanded(
          child: YaruMasterDetailPage(
          ```
          
          ?
          
          Well i don't know how to get this layout otherwise. Should I wrap it in a scaffold?

@jpnurmi
Copy link
Member Author

jpnurmi commented Feb 26, 2023

I'm still trying to understand the requirements. :)

The original question was how to query MD orientation and this PR adds that mechanism but I'm afraid it's not possible to access an inherited widget from where you'd like to, above the master detail page.

We could make it possible to query the current orientation from the controller, though.

class YaruMasterDetailController extends YaruPageController {
  Orientation get orientation => ...
}

@Feichtmeier
Copy link
Member

I'm still trying to understand the requirements. :)

The original question was how to query MD orientation and this PR adds that mechanism but I'm afraid it's not possible to access an inherited widget from where you'd like to, above the master detail page.

We could make it possible to query the current orientation from the controller, though.

class YaruMasterDetailController extends YaruPageController {
  Orientation get orientation => ...
}

Sounds enough for my case. But is this layout really so weird? 😄 Alternatively I could maybe wrap it in a scaffold?

@jpnurmi
Copy link
Member Author

jpnurmi commented Feb 26, 2023

It's not the layout being weird or you doing anything wrong. We definitely want to support the layout you have. It's just impossible in Flutter to access an inherited widget from the context above so we need to make it possible to get the information in some other way.

Scaffold(
  appBar: YaruWindowTitleBar(
    // context OUTSIDE YaruMasterDetailPage
    // - Navigator.of(context) finds the ROOT navigator, not the MD navigator
    // - YaruMasterDetailPage.of(context) doesn't find anything
  ),
  body: YaruMasterDetailPage(
    pageBuilder: (context, index) {
      // context INSIDE YaruMasterDetailPage
      // - Navigator.of(context) finds the MD navigator
      // - YaruMasterDetailPage.of(context) is OK
    },
  ),
)

@Feichtmeier
Copy link
Member

ah right!
I test this PR now

@Feichtmeier
Copy link
Member

@Feichtmeier does this help with the back button issue? i have a feeling it doesn't because if Navigator.of(context).canPop() didn't work for you, it sounds like you're building the title bar outside the master-detail page so you can't query the MD orientation via an inherited widget like this either?

confirming. as the element is not reachable from down there
Thanks for trying though! If this is still an improvement you can ofc still merge as you like

@jpnurmi
Copy link
Member Author

jpnurmi commented Mar 6, 2023

@Jupi007 @Feichtmeier do we have any use case for this?

@Jupi007
Copy link
Member

Jupi007 commented Mar 6, 2023

Imo, this can be useful in some situations.
Ex: I you want to make something different in the detail view depending on the layout.

@jpnurmi
Copy link
Member Author

jpnurmi commented Mar 6, 2023

Thanks for chiming in. I started feeling somehow unsure because this didn't solve the original issue. :)

@jpnurmi jpnurmi merged commit c8f3331 into ubuntu:main Mar 6, 2023
@jpnurmi jpnurmi deleted the md-app-bar-builder branch March 6, 2023 16:59
@jpnurmi jpnurmi mentioned this pull request Mar 11, 2023
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.

None yet

3 participants