Study circle detail page

[mikolaj-jalocha]

#33

Implemented page that shows details of particural study circle. Along with nested navigation in home view.

[simon-the-shark]

Nice work. Few things noted. Don't be scared when you use all those long code blocks - usually changes are only in few lines - the examples are just long.

[simon-the-shark]

There are few ways to avoid extra wrapping of custom app bar with PrefferedSizedWidget.

The way I usually do this is extending AppBar, instead of StatelessWidget (another way is to implement PrefferedSizedWidget in this custom widget, but you still need to define pref size). And when extending AppBar, we don't need to define it size. (To be honest, its definition is same as yours preffered size widget, but we just don't need to do it second time and by ourselves)

 appBar: DetailsScreenAppBar(
            context,
            title: context.localize?.study_circles ?? '',
          ),

class DetailsScreenAppBar extends AppBar {
  DetailsScreenAppBar(BuildContext context, {super.key, required String title})
      : super(
          automaticallyImplyLeading: false,
          title: TextButton(
              onPressed: () {
                Navigator.pop(context);
              },
              style: TextButton.styleFrom(
                padding: EdgeInsets.zero,
              ),
              child: Text(
                '< $title',
                style: context.textTheme.boldBodyOrange,
              )),
        );
}

It still works and behaves just the same, but I just think its clearer.

[mikolaj-jalocha]

Indeed, much cleaner. Implemented as proposed

[simon-the-shark]

The Scaffold and your custom app bar is repeated here, and it can be avoided. Two possibilities:

1. You can just move the switch 2 steps down the three:

class _StudyCircleDetailsLoading extends StatelessWidget {
  const _StudyCircleDetailsLoading();

  @override
  Widget build(BuildContext context) {
    return Shimmer(
      linearGradient: shimmerGradient,
      child: ListView(
        physics: const NeverScrollableScrollPhysics(),
        children: const [
          HeaderSectionLoading(),
          SizedBox(height: DetailsScreenConfig.spacerHeight),
          ContactSectionLoading(),
          SizedBox(height: DetailsScreenConfig.spacerHeight),
          AboutUsSectionLoading()
        ],
      ),
    );
  }
}

class StudyCircleDetails extends ConsumerWidget {
  const StudyCircleDetails({super.key});

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final itemId = ModalRoute.of(context)?.settings.arguments as String;
    final state = ref.watch(studyCircleRepositoryProvider(itemId));

    return Scaffold(
      appBar: DetailsScreenAppBar(
        context,
        title: context.localize?.study_circles ?? '',
      ),
      body: switch (state) {
        AsyncLoading() => const _StudyCircleDetailsLoading(),
        AsyncError(:final error) => MyErrorWidget(error),
        AsyncValue(:final value) => ListView(
            children: [
              HeaderSection(

2. Or simply extract one more widget: (this is slighty better, cause won't rebuild scaffold and app bar whenever provider changes - but not a change that is noticeable)

class StudyCircleDetails extends StatelessWidget {
  const StudyCircleDetails({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: DetailsScreenAppBar(context,
          title: context.localize?.study_circles ?? ''),
      body: const _CircleDetailsDataView(),
    );
  }
}

class _CircleDetailsDataView extends ConsumerWidget {
  const _CircleDetailsDataView();

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final itemId = ModalRoute.of(context)?.settings.arguments as String;
    final state = ref.watch(studyCircleRepositoryProvider(itemId));
    return switch (state) {
      AsyncLoading() => const _StudyCircleDetailsLoading(),
      AsyncError(:final error) => MyErrorWidget(error),
      AsyncValue(:final value) => ListView(
          children: [
           // ... contents
          ],
        ),
    };
  }
}

class _StudyCircleDetailsLoading extends StatelessWidget {
  const _StudyCircleDetailsLoading();

  @override
  Widget build(BuildContext context) {
    return Shimmer(
      linearGradient: shimmerGradient,
      child: ListView(

You can choose whatever you prefer.

[mikolaj-jalocha]

Thank you, the 2nd option looks great. Imo optimalization matters even in slightest matters :)

[simon-the-shark]

I think, when we have a normal function body, its a bit clearer with old-fashioned if - but a matter of preferences I guess

@override
  Widget build(BuildContext context) {
    if (text.isEmpty) return const SizedBox.shrink();
    return Padding(
      padding: const EdgeInsets.all(24),
      child: Column(
//...

not forcing this one on you

[simon-the-shark]

Apart from what I've mentioned before: quick note that at least for me, the title is centered and in figma is not. The solution is to just add centerTitle:false

class DetailsScreenAppBar extends AppBar {
  DetailsScreenAppBar(BuildContext context, {super.key, required String title})
      : super(
          centerTitle: false, // add this I guess ?
          automaticallyImplyLeading: false,
          title: TextButton(
              onPressed: () {

Unless you've centered it on purpose, then all fine

[mikolaj-jalocha]

That's an interesting one. On an Android device, everything looks normal (text is not centered). Apparently, it's some iOS platform thing here. I have added the line that you mentioned, but I don't have the possibility to test it on an iOS device. So please check and let me know :)

[simon-the-shark]

I would move this model to some separate file maybe. Example location: lib\features\detail_screen\models\contact_section_data.dart

[mikolaj-jalocha]

Good idea! Implemented as proposed

[simon-the-shark]

Also recently, I've made a util class for launching urls. so you can use it:

LaunchUrlUtil.launch(url) // but the url here accepts as String, so change this as well for the widget

[mikolaj-jalocha]

Changed as proposed

[simon-the-shark]

I suppose you can use my recently madeScrollableLoaderBuilder (without calculating elements count):

SizedBox(
            height: MediaQuery.of(context).size.height / 10,
            child: ShimmerLoadingItem(
              child: ScrollableLoaderBuilder(
                mainAxisItemSize: 16,
                itemsSpacing: 4,
                scrollDirection: Axis.vertical,
                itemBuilder: (BuildContext context, int index) {
                  return Container(
                    height: 16,
                    width: MediaQuery.of(context).size.width,
                    decoration: BoxDecoration(
                      color: Colors.white,
                      borderRadius: BorderRadius.circular(16),
                    ),
                  );
                },
              ),
            ),
          ),

check if it works the same though.

[mikolaj-jalocha]

Changed as proposed. I didn't notice any changes in UI relative to the previous one.

[simon-the-shark]

Here you have probably forgotten to lock the scrolling:

                physics: const NeverScrollableScrollPhysics(),

[mikolaj-jalocha]

Yeah.. Thank you. Changed as proposed.

[simon-the-shark]

To be honest, nesting navigator inside of HomeScreen and only HomeScreen is a bit weird. We will want to start the same details screen from others tab. I would change it this way: (As I said, I'm not the best authority on the topic, but I think this will be fine):

1. Reverse everything in this file as it was previously
2. Create new file lib\navigator.dart with following content: (it's basically a bit refactored version of your navigator)

import 'package:flutter/cupertino.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';

import 'features/bottom_nav_bar/bottom_nav_bar.dart';
import 'features/details_screen/study_circle_details.dart';
import 'widgets/my_error_widget.dart';

class AppNavigator {
  final navigatorKey = GlobalKey<NavigatorState>();

  static Route<dynamic> onGenerateRoute(RouteSettings settings) {
    return CupertinoPageRoute(
      builder: switch (settings.name) {
        _Routes.home => (_) => const BottomNavBar(),
        _Routes.studyCircleDetails => (_) => const StudyCircleDetails(),
        _ => (_) => MyErrorWidget('Invalid route: ${settings.name}')
      },
      settings: settings,
    );
  }

  void navigateToStudyCircleDetails(String argument) {
    navigatorKey.currentState
        ?.pushNamed(_Routes.studyCircleDetails, arguments: argument);
  }
}

class _Routes {
  _Routes._(); //no constructor available

  static const home = '/';
  static const studyCircleDetails = 'study-circle-details';
}

final navigatorProvider = Provider<AppNavigator>((ref) {
  return AppNavigator();
});

3. Then in lib\main.dart, we can provide our navigator to be used as default one:

class MyApp extends ConsumerWidget {
  const MyApp({super.key});
  @override
  Widget build(BuildContext context, WidgetRef ref) {
    return MaterialApp(
      title: MyAppConfig.title,
      localizationsDelegates: AppLocalizations.localizationsDelegates,
      supportedLocales: AppLocalizations.supportedLocales,
      theme: ThemeData(extensions: const [AppTheme()]),
      debugShowCheckedModeBanner: false,
      onGenerateRoute: AppNavigator.onGenerateRoute, //this
      navigatorKey: ref.watch(navigatorProvider).navigatorKey, // and this
    );
  }
}

4. Then you can use navigation wherever provider's ref is available:

ref.watch(navigatorProvider).navigateToStudyCircleDetails(id)
// ref.watch or ref.read - depending on the context

I'm not entirely sure it's the best way possible, but I think this will be more standard and usefull (as it will be global navigator for all tabs)

[mikolaj-jalocha]

This solution is defenitely better than mine, thank you :-) (still searching for good tutorial on topic).

I've changed everything as proposed, but one problem arose:

After changing the navigator logic, bottom bar's not visible when details screen's open.

In Figma, mentioned screen has bottom bar with proper tab marked as selected:

Any idea how we can cope with this?

[simon-the-shark]

I guess we can ask during our next weekly if this nav bar down there in Figma is actually supposed to be there. Cause I personally find this a bit odd, so I haven't noticed it before. So it's a good catch from you. We can discuss in weekly if we want such a nav bar in these overlaying"detail views"

If we actually want the navbar there, then I guess some sort of custom navigator nesting might be the actual best solution for it. Probably something similar to your previous one, but with some refactoring like we did just now and we would need to nest it not only in the home tab (HomeScreen) but the whole view (I'm talking about BottomNavBar widget - the name is kinda misleading).

Cause if we follow the Figma to the letter, it changes the tab to this "rocket" icon as well, so we would need to nest it in the whole view and not only the home tab. And this way we won't repeat the code for every tab too.

[simon-the-shark]

I will play with it tomorrow (probably). Cause I have one idea, but need to test it now

[simon-the-shark]

I've left note in the bottom of pr

[simon-the-shark]

I guess, after above changes, you can delete onNavigate and use zef.watch(navigatorProvider).navigateSMth directly here (?)

[mikolaj-jalocha]

changed as proposed

[simon-the-shark]

Also, I've forgot to add one thing in the end.

Pretty standard effect in such details views is when this background image and logo at the top, when scrolled down, it shrinks to simple app bar (or dissapears).

This is not a requested change - we can defienetly merge without it, but I think we can add this feature as TODO (we can discuss it in the weekly beforehand - but I think it will be nice in all details views with similar layout as this).

It is very easily achieavable in Flutter with CustomScrollView and SliverAppBar:
https://www.youtube.com/watch?v=G6qJkjt08k0
https://api.flutter.dev/flutter/material/SliverAppBar-class.html

• and many many more tutorials on the net.

Below are very simple example gifs from https://medium.com/@omer28gunaydin/flutter-sliverappbar-widget-fb5f4f30cf62:

but again - don't need to worry about this now. We can merge without it and this is potential future TODO task

[mikolaj-jalocha]

Also, I've forgot to add one thing in the end.

Pretty standard effect in such details views is when this background image and logo at the top, when scrolled down, it shrinks to simple app bar (or dissapears).

This is not a requested change - we can defienetly merge without it, but I think we can add this feature as TODO (we can discuss it in the weekly beforehand - but I think it will be nice in all details views with similar layout as this).

It is very easily achieavable in Flutter with CustomScrollView and SliverAppBar: https://www.youtube.com/watch?v=G6qJkjt08k0 https://api.flutter.dev/flutter/material/SliverAppBar-class.html

• and many many more tutorials on the net.

Below are very simple example gifs from https://medium.com/@omer28gunaydin/flutter-sliverappbar-widget-fb5f4f30cf62:

but again - don't need to worry about this now. We can merge without it and this is potential future TODO task

Good idea! I will create separate TODO for this and cope with it later :)

[simon-the-shark]

@mikolaj-jalocha I've pushed a commit with what I've managed to achieve. Now we have a nested navigator with an overlaying bottom nav bar. I think it works pretty fine. Check it out and tell me what you think of this solution.

Nevertheless, we still can discuss this on a weekly, how exactly we want this navigation and bottom nav bar to work. I see many possible questions about a specific behavior when we click that or this or even whether we want to see this nav bar in detail views or not. So I think we can discuss this with more people.

But if you think this will work ok, then we can merge with this now and after a discussion, we'll have another TODO task to correct some potential behavior changes (or just revert my commit if we'll decide to drop this nav bar in detail views)

[mikolaj-jalocha]

@mikolaj-jalocha I've pushed a commit with what I've managed to achieve. Now we have a nested navigator with an overlaying bottom nav bar. I think it works pretty fine. Check it out and tell me what you think of this solution.

Nevertheless, we still can discuss this on a weekly, how exactly we want this navigation and bottom nav bar to work. I see many possible questions about a specific behavior when we click that or this or even whether we want to see this nav bar in detail views or not. So I think we can discuss this with more people.

But if you think this will work ok, then we can merge with this now and after a discussion, we'll have another TODO task to correct some potential behavior changes (or just revert my commit if we'll decide to drop this nav bar in detail views)

Thanks for your support here! I've simillar view in this regard as yours. In my opinion previous behavior without bottom nav bar within details screen or the one that didn't change actual tab were working fine. Nevertheless I think we can merge with this feature for the Figma's sake :) and discuss the matter later and take appropraite steps