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

Feat/gh 357/account information screen #374

Open
wants to merge 16 commits into
base: development
Choose a base branch
from

Conversation

CaglarKullu
Copy link

I have created the information page:

  • Updating the user name section works properly
  • Need design for country change but I have created a bloc for that
  • After the back-hand of an email I can build it too

Notes: I couldn't find any bugs but need to review

@CaglarKullu CaglarKullu requested review from wizlif and Xazin March 16, 2023 18:54
Copy link
Member

@Xazin Xazin left a comment

Choose a reason for hiding this comment

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

I need to look through it more in depth at some point this week.

final IProfileRepository _profileRepository;
PersonalInformationBloc(this._profileRepository) : super(_Fetching()) {
_profileRepository.getUserProfile().then((profileOrFailure) {
profileOrFailure.fold((l) {}, (profile) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should handle the error state as well.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how we should deal with this error, since you shouldn't be able to reach the Account Information screen without being logged in, and thus having a profile.

What do you think is best @wizlif ?

I see these few options:

  • Create an anonymous version of the Widget that uses the profile (We'll use that as an error widget)
  • Show loading state (We'll implement a shimmer version), and a snackbar with an error

lib/domain/core/location.dart Outdated Show resolved Hide resolved
mainAxisAlignment: MainAxisAlignment.spaceBetween,
children: [
Text(
'${location?.name ?? "No"}, ${location?.code ?? "Country"}',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Create a getter in the location model to do this.

@CaglarKullu CaglarKullu requested review from wizlif and Xazin April 14, 2023 18:21
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