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

Release/homework 4 #8

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

Release/homework 4 #8

wants to merge 45 commits into from

Conversation

PhilipPeric
Copy link
Owner

No description provided.

@medvalna
Copy link

Привет!
Оценка работы по критериям:

  1. Readme соответствует критериям +1 балл
    2)flutter_lints добавлен, но есть игнор правил +0.5 баллов
  2. форматирование кода присутствует
  3. код разделен на слои + 2 балла
    5)инкапсуляции навигации нет +0 баллов
    6)обрезка заметок по макету есть +1 балл
  4. ночной темы нет +0 баллов
  5. анимация для добавления и редактирования заметок есть +1 балл
  6. поддержки landscape-ориентации нет +0 баллов
  7. поддержка больших экранов есть +2 балла
  8. работа с remote configs есть +2 балла
  9. работа с firebase crachlitics есть +1 балл
  10. работа с флейворами есть +2 балла
  11. CI на github есть + 4 балла
  12. работа с firebase app distribution есть + 2 балла
  13. работа с firebase analytics есть + 1 балл
  14. пакет freezed для дата-моделей не используется + 0 баллов.

Итог: 20.5 баллов

@PhilipPeric
Copy link
Owner Author

Привет:) Спасибо за ревью, все по делу

super.initState();
_getData();
}

@override
Widget build(BuildContext context) {
stringColor = _remoteConfig.getBool('default_importance_color');
return Scaffold(
backgroundColor: const Color(0xFFF7F6F2),
Copy link

Choose a reason for hiding this comment

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

Если ты используешь дефолтную тему ThemeData, то переорпделять ничего не нужно было, получается, что при темной теме, цвет скаффолда остается такой же

Comment on lines +33 to +34
theme: ThemeData(useMaterial3: true),
darkTheme: ThemeData.dark(useMaterial3: true),
Copy link

Choose a reason for hiding this comment

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

По сути это не есть поддержка темной темы, потому что большинство цветов виджетов просто захардкожены, а те что поменяли цвет, это те виджеты, которым не был задан конкретный цвет(использоватся цвет из дефолтных тем)

Нужно было либо не хардкодить каждый цвет и просто этого кода было бы достаточно, либо написать свой геттер темы, который через copyWith будет переопределять дефолтные цвета темной или светлой темы

@aa1dar
Copy link

aa1dar commented Jul 20, 2023

Привет, вот сколько баллов я насчитал:

  • Чистота кода и общая структура проекта: 4.5 балла(-0.5 за необоснованный игнор в FormExmaple)

  • Навигация 0 баллов

  • текст длинных заметок +1 балл

  • ночная тема нет - 0 баллов

  • поддержка лендскейп - 0 баллов:
    Как пример, не могу изменить последнюю задачу, т.к. кнопка закрывает её
    image
    Слишком большой раскрывающийся список:
    image

  • анимация +1 балл

  • поддержка больших экранов: +2 балла

  • FIrebase Remote Configs +2 балла

  • Firebase Crashlytics +1 балл

  • Build & Release +8 баллов:
    Есть замечание которое по критериям проходит, но в реальном проекте может нанести ущерб - это файлы с ключами: env.g.dart там по сути лежит обфуцированный ключ, но если я запущу проект могу спокойной его восстановить, его нужно было добавить в .gitignore
    Тоже самое касается всех файлов конфигурации firebase: firebase_options.dart, goolge-services.json.
    В CI эти файлы можно было бы получить из секретов перед этим добавив их туда.

  • Разное: 0 баллов (Firebase Analytics и Freezed не используется)
    Итого: 19.5 баллов

P.S. Если с чем-то не согласен, напиши, обсудим

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.

3 participants