Skip to content

Information was added#1

Open
user-nazar wants to merge 2 commits intomainfrom
information-button
Open

Information was added#1
user-nazar wants to merge 2 commits intomainfrom
information-button

Conversation

@user-nazar
Copy link
Owner

@user-nazar user-nazar commented Sep 27, 2021

There are some issues with starting the emulator. Therefore, I used a browser to launch the application.

This screen shows simple text output:

image

image

Copy link
Collaborator

@romanoprid romanoprid left a comment

Choose a reason for hiding this comment

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

I think it is right.

@user-nazar user-nazar self-assigned this Sep 27, 2021
alreadySaved ? Icons.favorite : Icons.favorite_border,
color: alreadySaved ? Colors.red : null,
alreadySaved ? Icons.done : Icons.attach_money,
color: alreadySaved ? null : Colors.green,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure, but if the user taps to save some startup name, the icon will disappear, am I right?
If so, I would instead give the user a possibility to discard their choice, for example.

Choose a reason for hiding this comment

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

I don't recommend using null for colors ever :)

),
body: const Center(
child: Text(
"Here you can add your words to the cart. \nYes!!! You can buy them. \n The future is now, wake up",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't understand the purpose of this page. You can add words to the cart on the first (main) page, but you claim to do the same thing on the info page.
The idea is cool but needs some tuning.

P.S. It would be better to save your text to the constant and then use that constant.

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 the purpose of this page is to inform users what is on the 'Saved Suggestions' page but not to add words.

Copy link
Owner Author

Choose a reason for hiding this comment

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

In another pull of requests, I expanded the functionality of the application

Choose a reason for hiding this comment

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

I wouldn't recommend splitting a single feature into multiple PRs in case this feature is small (like yours).

child: Text(
"Here you can add your words to the cart. \nYes!!! You can buy them. \n The future is now, wake up",
style: TextStyle(
fontSize: 14, color: Colors.black, fontWeight: FontWeight.normal),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't text have a normal font weight by default?

Choose a reason for hiding this comment

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

I'm sure it does (but @Nazarko12 correct me if I'm wrong)

Copy link

@igor-leshkevych igor-leshkevych left a comment

Choose a reason for hiding this comment

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

@Nazarko12

your feature is a bit "chaotic" (honestly, it was hard for me to understand it), but nevertheless it wasn't a requirement for this lab :)

+5/5 hello world
+3/3 custom feature + PR

Unfortunately, I don't see any code review from you, so you have 0/2 for that.

alreadySaved ? Icons.favorite : Icons.favorite_border,
color: alreadySaved ? Colors.red : null,
alreadySaved ? Icons.done : Icons.attach_money,
color: alreadySaved ? null : Colors.green,

Choose a reason for hiding this comment

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

I don't recommend using null for colors ever :)

),
body: const Center(
child: Text(
"Here you can add your words to the cart. \nYes!!! You can buy them. \n The future is now, wake up",

Choose a reason for hiding this comment

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

I wouldn't recommend splitting a single feature into multiple PRs in case this feature is small (like yours).

child: Text(
"Here you can add your words to the cart. \nYes!!! You can buy them. \n The future is now, wake up",
style: TextStyle(
fontSize: 14, color: Colors.black, fontWeight: FontWeight.normal),

Choose a reason for hiding this comment

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

I'm sure it does (but @Nazarko12 correct me if I'm wrong)

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.

4 participants