Solvro / mobile-topwr

ToPWR mobile flutter application
https://solvro.pwr.edu.pl/portfolio/to-pwr
GNU Affero General Public License v3.0
14 stars 1 forks source link

Mobile 14 #43

Closed gry-mar closed 2 months ago

gry-mar commented 3 months ago

26

Implemented Research Group Tab according to UI v1.0.0 with redirection to Study Circles detail view.

gry-mar commented 2 months ago

I applied changes according to your CR and also changed the search query in scientific_circles_tab_repository. But I used force unwrapping which is not elegant but I didn't have an idea how to implement it differently in that case

@riverpod
Future<Iterable<ScientificCircle?>?> _sciCirclesFilteredByTextQuery(
    _SciCirclesFilteredByTextQueryRef ref) async {
  final originalList =
      await ref.watch(scientificCirclesRepositoryProvider.future);
  final query = ref.watch(searchScientificCirclesControllerProvider);
  return originalList?.where((element) =>
      element == null ||
      element.name.toLowerCase().contains(query.toLowerCase()) ||
      element.department == null ||
      element.department!.name.toLowerCase().contains(query.toLowerCase()));
}

Also I noticed that now there's a bug that if you open the tab there is "Wszystkie" selected but there aren't any circles shown. If you click on other tag and come back to "Wszystkie" they are all present. Like the view is not waiting for data while it renders. I have this issue after applying the changes and for now I don't know where is the problem as the ScientificCirclesDataView should render only if the circles list is present.

simon-the-shark commented 2 months ago

It's ok to use force unwrapping in this case, cause you manually check it before and there's no option this will cause an error:

element.department == null ||
      element.department!.name.toLowerCase().contains(query.toLowerCase())

if element.department is null, then the rest of the expression won't be evaluated, cause || works lazily.

However, if you want, then you can do it this way too:

element.department?.name.toLowerCase().contains(query.toLowerCase()) !=
          false

OR

element.department?.name.toLowerCase().contains(query.toLowerCase()) !=
          true

the behaviour will be different, you should use either use != true or != false, depending if you want treat null as false or as true

But as I said, force checks are ok if we manually check if the value is null or not with if, || or &&, so do what you want here

simon-the-shark commented 2 months ago

After above changes you can merge