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

Refactor: Community/User avatars #1070

Merged
merged 8 commits into from
Jan 19, 2024
Merged

Refactor: Community/User avatars #1070

merged 8 commits into from
Jan 19, 2024

Conversation

hjiangsu
Copy link
Member

Pull Request Description

This PR performs a small refactor on the community and user avatars. Most notably:

  • Renamed CommunityIcon to CommunityAvatar to match naming of the UserAvatar
  • Moved CommunityAvatar and UserAvatar into shared/avatars directory
  • Added more comments and documentation into the respective widgets
  • Made community in CommunityAvatar, and person in UserAvatar non-nullable. Previously, the community and person parameters were nullable, but it was indicated as required in the constructor.

For example:

final Community? community;

const CommunityIcon({super.key, required this.community, this.radius = 12.0});

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@micahmo
Copy link
Member

micahmo commented Jan 19, 2024

  • Made community in CommunityAvatar, and person in UserAvatar non-nullable. Previously, the community and person parameters were nullable, but it was indicated as required in the constructor.

I think we want to support a nullable community! For example, on the create post page before selecting a community, we want to have an "empty" avatar to fill the space. Otherwise it looks funny!

I think instead of making it non-nullable, it could be not required by the constructor.

image

@hjiangsu
Copy link
Member Author

For example, on the create post page before selecting a community, we want to have an "empty" avatar to fill the space. Otherwise it looks funny!

Good check! I reverted the change and removed required from the constructor

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

LGTM!

@hjiangsu hjiangsu merged commit 37ee89b into develop Jan 19, 2024
1 check passed
@hjiangsu hjiangsu deleted the refactor/avatars branch January 19, 2024 19:35
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