Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Lp 188 implement update UI#77

Open
cpolat-tgm wants to merge 16 commits intoLP-156-Frontend-Refactorfrom
LP-188-implement-update-ui
Open

Lp 188 implement update UI#77
cpolat-tgm wants to merge 16 commits intoLP-156-Frontend-Refactorfrom
LP-188-implement-update-ui

Conversation

@cpolat-tgm
Copy link

@cpolat-tgm cpolat-tgm commented Mar 12, 2024

This change is Reviewable

@github-actions
Copy link

Analysis Report for f4e6fb7

  • Infos: 5
  • Warnings: 0
  • Errors: 0
Click to see the full report
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:66:60 • comment_references
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:72:60 • comment_references
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:78:60 • comment_references
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:90:90 • comment_references
info • Use 'const' literals as arguments to constructors of '@immutable' classes • lib/shared/presentation/widgets/sidebar.dart:58:31 • prefer_const_literals_to_create_immutables

View annotated files

Copy link
Collaborator

@mcquenji mcquenji left a comment

Choose a reason for hiding this comment

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

Is the height of the instructions dialog with the MarkdownView still hardcoded? Or is it dynamic?

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @cpolat-tgm)


lib/app_router.dart line 119 at r1 (raw file):

            page: UpdateRoute.page,
            path: '/update',
            initial: true,

set login as initial route


lib/features/update/presentation/screens/update_screen.dart line 7 at r1 (raw file):

/// Renders an update screen, which allows the user to update the app.

remove unnecessary newline


lib/features/update/presentation/widgets/widgets.dart line 1 at r1 (raw file):

//Export widgets

if there are no widgets remove this file


lib/shared/presentation/widgets/markdown.dart line 38 at r1 (raw file):

    return ConditionalWidget(
      condition: data == null,
      ifTrue: FutureBuilder<HttpResponse>(

you may want to wrap this widget in a builder to avoid unnecessary network traffic when no Uri was provided


lib/shared/presentation/widgets/markdown.dart line 42 at r1 (raw file):

        builder: (context, snapshot) {
          if (snapshot.hasData) {
            if (snapshot.data!.statusCode == 200) {

use HttpResponse.isOk instead of status code


lib/shared/presentation/widgets/markdown.dart line 58 at r1 (raw file):

                Text(context.t.widgets_markdown_networkError(source.toString()),
                    textAlign: TextAlign.center,
                    style: TextStyle(fontSize: 12, letterSpacing: 0.4)),

please add a comma, for propper formatting


lib/shared/presentation/widgets/screen_title_bar.dart line 74 at r1 (raw file):

                height: profileImageSize,
                child: CircleAvatar(
                  backgroundImage: NetworkImage(

why not use an icon or an image from assets instead of an online image? Unnecessary network traffic

@github-actions
Copy link

Analysis Report for a261752

  • Infos: 5
  • Warnings: 0
  • Errors: 1
Click to see the full report
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:66:60 • comment_references
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:72:60 • comment_references
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:78:60 • comment_references
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:90:90 • comment_references
error • Target of URI doesn't exist: 'widgets/widgets.dart' • lib/features/update/presentation/presentation.dart:1:8 • uri_does_not_exist
info • Use 'const' literals as arguments to constructors of '@immutable' classes • lib/shared/presentation/widgets/sidebar.dart:58:31 • prefer_const_literals_to_create_immutables

View annotated files

@github-actions
Copy link

Analysis Report for 856b713

  • Infos: 5
  • Warnings: 0
  • Errors: 0
Click to see the full report
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:66:60 • comment_references
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:72:60 • comment_references
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:78:60 • comment_references
info • The referenced name isn't visible in scope • lib/features/notifications/domain/models/notification.dart:90:90 • comment_references
info • Use 'const' literals as arguments to constructors of '@immutable' classes • lib/shared/presentation/widgets/sidebar.dart:58:31 • prefer_const_literals_to_create_immutables

View annotated files

Copy link
Collaborator

@mcquenji mcquenji left a comment

Choose a reason for hiding this comment

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

Is the height of the instructions dialog with the MarkdownView still hardcoded? Or is it dynamic?

?

Reviewed 2 of 4 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: 20 of 21 files reviewed, all discussions resolved (waiting on @cpolat-tgm)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants