Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor: Extract AppVersionText into a separate widget + Add a new widget = ChipText #251

Merged
merged 6 commits into from
Apr 5, 2022

Conversation

wal33d006
Copy link
Contributor

No description provided.

@wal33d006 wal33d006 changed the title refactor: Extract AppVersionInfo into a separate widget refactor: Extract AppVersionText into a separate widget Apr 5, 2022
@wal33d006 wal33d006 requested review from andrzejchm and Zfinix April 5, 2022 04:33
@wal33d006 wal33d006 changed the title refactor: Extract AppVersionText into a separate widget refactor: Extract AppVersionText into a separate widget + Add a new widget = ChipText Apr 5, 2022
Copy link
Contributor

@andrzejchm andrzejchm left a comment

Choose a reason for hiding this comment

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

looks great! lets just add some optional styling capabilities to the widgets

import 'package:flutter/material.dart';

class AppVersionText extends StatelessWidget {
const AppVersionText({required this.appInfoProvider, Key? key}) : super(key: key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const AppVersionText({required this.appInfoProvider, Key? key}) : super(key: key);
const AppVersionText({required this.appInfoProvider, Key? key,}) : super(key: key);

final AppInfoProvider appInfoProvider;

@override
void debugFillProperties(DiagnosticPropertiesBuilder properties) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this method should be at the bottom of the class

@override
Widget build(BuildContext context) => FutureBuilder<String>(
future: appInfoProvider.getAppVersion(),
builder: (context, snapshot) => Text(snapshot.data ?? ''),
Copy link
Contributor

Choose a reason for hiding this comment

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

right now we're not able to style it, let's add another optional TextStyle? to the constructor of AppVersionText and pass it into this Text widget

padding: EdgeInsets.all(theme.spacingM),
decoration: BoxDecoration(
borderRadius: BorderRadius.circular(10),
color: theme.colors.chipBackground,
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make the Color? color an optional parameter to the ChipText widget with the default value of theme.colors.chipBackground

borderRadius: BorderRadius.circular(10),
color: theme.colors.chipBackground,
),
child: Text(title),
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add optional parameter of TextStyle? to the ChipText constructor and pass it to this Text widget

@wal33d006 wal33d006 requested a review from andrzejchm April 5, 2022 08:22
Copy link
Contributor

@andrzejchm andrzejchm left a comment

Choose a reason for hiding this comment

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

🌟

@andrzejchm andrzejchm merged commit 642f7f2 into main Apr 5, 2022
@andrzejchm andrzejchm deleted the app-version-info branch April 5, 2022 08:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants