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

fixed linking and formatting issues #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

angelinetu
Copy link
Owner

What's new in this PR

Description

added styles in styles.ts for the image, text, comments, etc.

Screenshots

How to review

Next steps

Relevant links

Online sources

Related PRs

CC: @ronniebeggs

@ronniebeggs ronniebeggs self-requested a review October 3, 2024 21:21
Copy link
Collaborator

@ronniebeggs ronniebeggs left a comment

Choose a reason for hiding this comment

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

Looking good so far! I've left a few comments to consider before tomorrow's workshop. See if you can utilize more flexbox properties to achieve our desired styles (practice this here: https://flexboxfroggy.com/)

Comment on lines 12 to 18
<View style={styles.profileHeader}>
<ProfilePlaceholder style={styles.profileIcon} />
<View>
<Text style={styles.profileName}>rbeggs</Text>
<Text style={styles.postDate}>September 19</Text>
</View>
</View>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that we want the username + profile placeholder to be grouped together, let's wrap them in their own div. Then apply justifyContent: 'space-between' to the profileHeader style to spread information across the width of the screen.

Comment on lines 10 to 11
<View style={styles.container}>
<ProfilePlaceholder />
<Text>rbeggs</Text>
<Text>September 19</Text>
<Text>
{/* Profile Header */}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider applying styles to add spacing between the content and the sides of the screen. You can do so with the padding prop applied to the container View.

Comment on lines 46 to 50
<View style={styles.interactionRow}>
<HeartIcon style={styles.icon} />
<Text style={styles.likesText}>256 Likes</Text>
<ShareIcon style={styles.icon} />
</View>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apply similar styles as the profileHeader suggestion to spread icons across the width of the screen.

Comment on lines +52 to +53
{/* Comments */}
<View style={styles.commentSection}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

One way to add a content divider would be to apply a border style to the top of the commentSection div.

Comment on lines 56 to 60
<View>
<Text style={styles.commentName}>philip_ye</Text>
<Text style={styles.commentText}>
This organization is doing amazing work tackling the complex root
causes of the issue.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How could we integrate the comment text here? Try placing the username and date text in the same div, that way they appear inline, above the comment text.

postText: {
fontSize: 14,
lineHeight: 20,
marginBottom: 10,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's avoid applying spacing styles to text.

Comment on lines 52 to 71
commentSection: {
marginTop: 20,
},
comment: {
flexDirection: 'row',
alignItems: 'center',
marginBottom: 10,
},
commentIcon: {
width: 30,
height: 30,
marginRight: 10,
},
commentName: {
fontWeight: 'bold',
},
commentText: {
fontSize: 14,
lineHeight: 18,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

When we define absolute widths and margins and place them in line with each other, they'll compete and push content off the side of the screen. Using flex boxes with rowGap and columnGap properties can remedy this issue.

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