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

[Feature] #567643 - new DSUserAvatar customization #246

Merged
merged 2 commits into from
Jan 5, 2024

Conversation

adrianoct42
Copy link
Collaborator

@adrianoct42 adrianoct42 commented Jan 4, 2024

This PR aims to add new style options to the DSUserAvatar widget in the app.
A sample file was also created to assure the new style feature can work with the afore mentioned Avatar.

@adrianoct42 adrianoct42 requested a review from mpamaro January 4, 2024 20:06
@adrianoct42 adrianoct42 self-assigned this Jan 4, 2024
Copy link
Collaborator

@mpamaro mpamaro left a comment

Choose a reason for hiding this comment

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

Really nice job! I've left some suggestions to improve your code 🚀

const SizedBox(
height: 15,
),
DSHeadlineExtraLargeText(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could put this one (and the above) inside the already existing SampleTextStyleShowcase widget.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@@ -331,6 +331,7 @@ class DSCard extends StatelessWidget {
style: style,
shouldAuthenticate: shouldAuthenticate,
isUploading: isUploading,
// Devo criar um parametro novo no DSFileMessageBubble que será o texto da mensagem? Só depois instanciá-lo aqui no caso.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that you forgot to remove this comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

super.fontStyle,
super.shouldLinkify,
super.isSelectable,
super.height = 1.4,
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to Design specifications, this height should be 1.25.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

super.fontStyle,
super.shouldLinkify,
super.isSelectable,
super.height = 1.4,
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to Design specifications, this height should be 1.25.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

import '../utils/ds_font_weights.theme.dart';
import 'ds_text_style.theme.dart';

/// A Design System's [TextStyle] primarily used by large titles.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update this comment to [...] used by extra large titles.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

});
this.textColor = DSColors.neutralDarkCity,
TextStyle textStyle = const DSBodyTextStyle(),
}) : textStyle = textStyle.apply(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since TextStyle already has a color parameter, you could get rid of the textColor prop to avoid duplicity. Then, my suggestion would be to use the Merge method from TextStyle so you can assure the usage of default values in case they aren't passed through textStyle prop:

static const _defaultTextStyle = TextStyle(
    color: DSColors.neutralDarkCity,
    overflow: TextOverflow.clip,
  );
: textStyle = _defaultTextStyle.merge(textStyle)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@adrianoct42 adrianoct42 merged commit 47037e0 into develop Jan 5, 2024
1 check passed
@adrianoct42 adrianoct42 deleted the feature/567643-extra-large-style branch January 5, 2024 15:55
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.

2 participants