Skip to content

Fahad AlQarni - Task-2T2#1

Open
fahadasq wants to merge 1 commit intoGDSC-IAU:masterfrom
fahadasq:master
Open

Fahad AlQarni - Task-2T2#1
fahadasq wants to merge 1 commit intoGDSC-IAU:masterfrom
fahadasq:master

Conversation

@fahadasq
Copy link

Read README.md

Comment on lines +15 to +24
String notificationTypeToString(NotificationType type) {
switch (type) {
case NotificationType.like:
return "liked";
case NotificationType.dislike:
return "disliked";
case NotificationType.favorite:
return "favorited";
}
}

Choose a reason for hiding this comment

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

You can return switch expression without needing to return in each case:

String notificationTypeToString(NotificationType type) {
  return switch (type) {
    NotificationType.like => "liked",
    NotificationType.dislike => "disliked",
    NotificationType.favorite => "favorited"
  };
}

Comment on lines +3 to +16
class DefaultColors {
static const Color profileCardColor = Color.fromARGB(255, 76, 73, 86);

static const Color fieldColor = Color.fromARGB(255, 47, 45, 56);
static const Color fieldOutlineColor = Color.fromARGB(255, 75, 74, 83);

static const Color blockColor = Color.fromARGB(255, 39, 35, 43);

// This is done for convenience's sake, I find that matching the blocks outline with the field color gives a nice look
// so this is just a way for me to avoid having to reset both the block outline and the field color each time I change it
static const Color blockOutlineColor = fieldColor;

static const Color userHandleColor = Color.fromARGB(255, 156, 156, 156);
}

Choose a reason for hiding this comment

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

Nice idea to gather colors in one place.

Comment on lines +6 to +20
class FahadPosts {
static const PostModel persona = PostModel(
"The new Persona 3 remake is AWESOME",
user: Users.fahad,
likes: 6,
favorites: 2,
dislikes: 1,
imageAttachment: AssetImage("assets/posts/persona.jpg"),
);
static const PostModel flutter = PostModel(
"Learning flutter's been pretty cool",
user: Users.fahad,
likes: 2,
);
}

Choose a reason for hiding this comment

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

Good idea to separate the posts, but I recommend you create a field in UserModel with name posts and attach each user posts with it, with that you can access the posts easily for time being.

Read about encapsulation
https://en.wikipedia.org/wiki/Encapsulation_(computer_programming)

Copy link
Author

Choose a reason for hiding this comment

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

So just a little commentary on this.

I remember now that I had initially thought of this when I was writing the code, but I thought that it would be an unnecessary performance (specifically memory) overhead to associate each UserModel with a list of posts that will probably not be used for the use context of UserModel (assuming we do this by having a List<PostModel> field in the UserModel class). Things like post reactions, as well as individual replies under certain posts don't need to load every post of that user in memory, when it's overwhelmingly likely that most of it is not needed nor will it be used.

In retrospect, this issue would only really be an issue if it isn't the case that the posts variable wouldn't necessarily be the raw contents of the posts, but something that points to the list of posts. Such that, once we load any given UserModel variable, we're not loading all the posts, but we're loading the pointer or access point, if you will, of the list of posts. I don't know how Dart works under the hood, but it's probably likely that this is how it's done since almost all languages allocate the contents of dynamic arrays on the heap, and not in the same area where the List variable actually exists.

So your approach is probably right, but I just seemed to have overthought the performance implications of having a List variable inside a class in Dart, and overlooked how dynamic arrays work for most languages :)

If you're interested more in this topic, here are some great resources for learning about it:

And of course, thank you Hassan and Arwa for your leadership of the GDSC dev unit :)

@@ -0,0 +1,43 @@
import 'package:flutter/material.dart';

BoxDecoration roundedRectangle(Color color,

Choose a reason for hiding this comment

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

Good to factor a common decorations.

Comment on lines +95 to +173
Container(
height: 60,
alignment: Alignment.center,
child: Column(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
mainAxisSize: MainAxisSize.max,
children: [
const Icon(
Icons.comment_outlined,
size: 30,
),
Text(
(post.replies == null) ? "0" : "${post.replies}",
style: const TextStyle(fontSize: 13),
),
],
),
),

// Likes
Container(
height: 60,
alignment: Alignment.center,
child: Column(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
mainAxisSize: MainAxisSize.max,
children: [
const Icon(
Icons.thumb_up_outlined,
size: 30,
),
Text(
"${post.likes}",
style: const TextStyle(fontSize: 13),
),
],
),
),

// Dislikes
Container(
height: 60,
alignment: Alignment.topCenter,
child: Column(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
mainAxisSize: MainAxisSize.max,
children: [
const Icon(
Icons.thumb_down_outlined,
size: 30,
),
Text(
"${post.dislikes}",
style: const TextStyle(fontSize: 13),
),
],
),
),

// Favorites
Container(
height: 60,
alignment: Alignment.topCenter,
child: Column(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
mainAxisSize: MainAxisSize.max,
children: [
const Icon(
Icons.favorite_outline,
size: 30,
),
Text(
"${post.favorites}",
style: const TextStyle(fontSize: 13),
),
],
),
),
],

Choose a reason for hiding this comment

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

You can extract a common widget (recommended) or method.

Comment on lines +65 to +73
Navigator.push(
context,
MaterialPageRoute(
builder: (context) => SearchResultsPage(
searchResults: searchResults,
searchCriteria: value,
),
),
);

Choose a reason for hiding this comment

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

You can use Dialog to pop up a search results.
https://api.flutter.dev/flutter/material/Dialog-class.html

@alashoor89
Copy link

All other things are good, keep going.
You will continue use your code in the future tasks in sha allah.

Review the comments and stay tuned.

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