Closed arroyoj closed 1 year ago
There is currently a big Pull Request in the works that reworks UI completely as i understood: https://github.com/Teifun2/nextcloud-cookbook-flutter/pull/184
@Leptopoda would i make sense to wait for your rework befor individual screens are reworked?
I don't really know.
I think this patch is a bit useless as I restructured the entire base this PR uses.
All Three options require this feature being implemented again so I don't know. Although I'd appreciate getting feedback to my current changes. I don't use a tablet and the redesign will be the place I can have a look at everything.
Thank you both for the comments, and sorry I didn't realize that such a big rewrite was underway. Even if this PR needs to be re-implemented after #184, it would be great to know whether the landscape layout as demonstrated in the screenshots would be accepted or not, or needs reworking (the design more than the code itself, given the pending rewrite).
I took a quick look at the current state of #184. Here is the section that covers what my PR touches (the layout of the tools/nutrition/ingredients/instruction): https://github.com/Leptopoda/nextcloud-cookbook-flutter/blob/a5b022443ab783e1365fe39a318c097c7715288a/lib/src/screens/recipe/widget/recipe_screen_body.dart#L193-L215. It looks like that rewrite already fixes the bug of the nutrition info taking up space even if the recipe has no nutrition info, so that's great. If we wanted to implement the same layout I have in this PR, I think something like this would work (this could probably be written better, but just to demonstrate):
Widget _buildBottom() {
return SliverPadding(
padding: const EdgeInsets.only(bottom: 75.0),
sliver: SliverList(
delegate: SliverChildListDelegate.fixed([
if (MediaQuery.of(context).size.width > 600) ...[
Row(
crossAxisAlignment: CrossAxisAlignment.start,
children: [
if (recipe.tool.isNotEmpty)
Expanded(
flex: 5,
child: _ToolList(recipe),
),
if (recipe.nutritionList.isNotEmpty)
Expanded(
flex: 5,
child: _NutritionList(recipe.nutritionList)
),
]
),
Row(
crossAxisAlignment: CrossAxisAlignment.start,
children: [
if (recipe.recipeIngredient.isNotEmpty)
Expanded(
flex: 5,
child: _IngredientList(recipe),
),
Expanded(
flex: 10,
child: _InstructionList(recipe)
),
]
),
]
else ...[
if (recipe.tool.isNotEmpty) _ToolList(recipe),
if (recipe.nutritionList.isNotEmpty) _NutritionList(recipe.nutritionList),
if (recipe.recipeIngredient.isNotEmpty) _IngredientList(recipe),
_InstructionList(recipe),
]
]),
),
);
}
Anyway, happy to coordinate with #184.
@arroyoj Generally i think your change is deffinitvely the correct way to go, in terms of how the content is arranged / not showing empty containers etc.
I think it would be best if we wait for the rework to hit and then if you have time, you could adapt the PR to match the new design.
If not maybe somone of us can pick it up. Would that be fine for you?
@Teifun2, thanks, that sounds like a good plan. I'll wait until #184 is merged and then re-implement the design from there.
ping.
@arroyoj you can now rebase :)
I'll have a look at it shortly but a quick glance at the diff looks good.
Just out of curiosity are there any other optimizations we could make to tablet layouts that you can think of? With the big redesign I'd love to get proper tablet support also done.
Ok I'll need to adjust the CI settings :)
@Leptopoda, thanks for the review.
Just out of curiosity are there any other optimizations we could make to tablet layouts that you can think of? With the big redesign I'd love to get proper tablet support also done.
Nothing I can think of right now, but I'll take a closer look at the new design on my tablet.
The current landscape layout uses 3 columns for nutrition info, ingredients, and instructions. On my tablet, this gives very little space for the instructions and can require a lot of scrolling for some recipes. In addition, even when a recipe does not contain nutrition info, the 3 columns are still used instead of hiding the nutrition area to provide more space for ingredients and instructions. Similar to @psleep in #161, I think a 2 column layout for ingredients and instructions would make the landscape view better and easier to use.
This PR moves the nutrition information up into the same row as the tools list when in landscape view, freeing up horizontal space for the ingredients and instructions. Since each instruction is usually longer than an ingredient, I made the instructions area twice the width of the ingredients list (and instructions become full width if there are no ingredients). If either the tools list or nutrition info is not present in the recipe, the corresponding area is not shown. This PR makes no changes to the portrait layout.
I'd welcome any feedback or suggestions for the new landscape layout. @psleep, is this what you were thinking of in your issue?
Here are a few before and after screenshots to show the changes:
Original landscape layout (same whether recipe has nutrition info or not)
New landscape layout - default after load
New landscape layout - tools / nutrition expanded
New landscape layout - no nutrition info