-
Notifications
You must be signed in to change notification settings - Fork 0
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
Styling user profile #70
base: native-development
Are you sure you want to change the base?
Conversation
…t' unsure where it is coming from will do more digging but MVP
…lso added the include a fun fact! back in
native/screens/UserProfile.jsx
Outdated
<Text>{fun_fact}</Text> | ||
<View style={styles.wordContainer}> | ||
<Text style={styles.portfolio } | ||
onPress={() => Linking.openURL(`${portfolio_link}`)}> Portfolio </Text> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is curious! I've never seen a text with an onpress or the Linking
function... very cool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a similar and this is not your fault, I don't know why there was a box for the Portfolio URL here on the figma. I don't think a simple link merits a whole box and I feel like it would be better located at the top of the screen near the user's name or something.
I'm going to bring this up to the UX team but we can delete this for now, I don't think it will survive that discussion
native/screens/UserProfile.jsx
Outdated
<Button | ||
title="Reject Request" | ||
onPress={() => { | ||
handleTextChange(true, 'renderDecisionModal', setReviewStatus); | ||
handleTextChange(false, 'ownerDecision', setReviewStatus); | ||
}} | ||
/> | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these look 1 tab indentation too deep, open and close brackets should be parellel
<Open bracket
-thing 1
-thing 2
-thing 3
/> close bracket
native/screens/UserProfile.jsx
Outdated
</View> | ||
|
||
<View> | ||
<Text>{about}</Text> | ||
<View style={styles.wordContainer}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed we created this container 4 times here. This is a good example of when to use the DRY principle by creating another component that will style and render information passed into it via props.
That way, instead of:
<View style={styles.wordContainer}>
<Text style={styles.title}>ABOUT</Text>
<Text style={styles.words}>{about}</Text>
</View>
4 times, we can have 1 component that applies all the styles and renders text based on what's passed in
const AboutBox = (text, title) => {
return (
<View style={styles.wordContainer}>
<Text style={styles.title}>{title}</Text>
<Text style={styles.words}>{text}</Text>
</View>
)
}
Rendered on UserProfile:
<AboutUser title="ABOUT" text="I love coding" />
<AboutUser title="FUN FACT" text="I'm a nerd" />
Converting 16 lines of code to just 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes the function won't work for all of them. For example when you created the View for the Portfolio
URL with a custom text component that needed an onPress
prop.
That's a one-off use case so it's okay to use as you are. However (and dont hate me bcs the idea just came to me), we can still make it work with custom component by just using that component as a container, instead of making it in charge of rendering the info as well. For example:
const const AboutBox = ({ children, text, title }) => {
return (
<View style={styles.wordContainer}>
<Text style={styles.title}>{title}</Text>
{children}
</View>
)
}
Which would look like this on UserProfile:
<AboutUser title="about" >
<Text>Still a nerd</Text>
</AboutUser>
<AboutUser title="about" >
<Text style={styles.portfolio } onPress={() => Linking.openURL(`${portfolio_link}`)}> Portfolio </Text>
</AboutUser>
While it's more verbose, it enables us to be able to use the AboutUser
component anywhere in the app if we want a box with that same styling
Don't worry if all this feels overwhelming!!! I have a tendency to share what's on my mind all the time but you're doing good! And this kind of exchange of information is super super common in the workplace and may come off like criticism but it's always in the interest of learning!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @staceycsikos Good work! You guys are picking up quick!
I know I gave you a lot to work with but to summarize for simplicity:
- a quick formatting on VSCode will resolve most of the conversations
- to stay DRY we should convert the styled boxes into a single component. Either approach I suggested would work, don't think I am not open to hearing your thoughts on them!
Then I had a few concerns:
- This PR includes a 13,000 line changes in the package.jsons and I'm curious why. It's hard to narrow down why there were such big changes to them so I'd like to debug that with you.
- The Screen does not exactly reflect the figma screen:
- There is a bit more padding around the title and texts
- The screen does not show the user's portfolio projects
On a separate note, this file is becoming very long and unnecessarily verbose/hard to read. It's been on my mind to refactor this file to be easier to work with for a while and I encourage you to take the challenge! I'm thinking we can relocate the helper components to a separate file and only leaving the UserProfile component here.
That will reduce a lot of the noise and leave only the core logic for the screen.
@@ -1,2618 +1,8 @@ | |||
{ | |||
"name": "bootcamper", | |||
"version": "1.0.0", | |||
"lockfileVersion": 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run a git pull origin native-development
…. still unsure what final user profile will look like if we will have projects listed there or not.
refactoring done, unsure about export const aboutme part
I noticed in the figma we have fun facts, but in our UserProfile comp we had email and portfolio link. I comment those out for now pending what we decide to have there.
For the edit button the icon I found on ionicons is not circular.