-
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
[cases] CaseScreen bugs #82
Conversation
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.
Everything's looking good so far! Just a few comments to address. I don't know how to mitigate the button feedback while scrolling but I'll ask david in the meantime.
<View style={styles.container}> | ||
<View style={styles.topContainer}> | ||
<Text style={styles.statusText}>Case Status:</Text> | ||
<TouchableOpacity | ||
style={styles.updatesButton} | ||
onPress={() => { | ||
router.push({ pathname: `AllCases/Updates/${caseUid}` }); | ||
}} | ||
> | ||
<TouchableOpacity | ||
style={styles.updatesButton} |
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 think it'd be cleaner to make this full container a button by switching the View and TouchableOpacity components. That way the outer container becomes a button with the container
styling, while the previous TouchableOpacity becomes a view with the updatesButton
styling. Make sure to change the names though!
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 ended up just removing the view div so all the styling for the button is in updatesButton and then textContainer and statusContainer define the styling for the content on the button.
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.
meow
…/calblueprint/impact-fund into arfamomin/imp-82-casescreen-bugs
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.
Looks good! I tried standardizing shadow styling to apply across the app. Feel free to check how I did that. Please just fix the util function and it should be ready to merge!
Looked over how you standardized the shadow styling and double checked if there was anywhere else to apply it -- saw the update view when you click into a specific update had similar styling except the border radius was 15. I applied it to that in my latest commit but if it's bad practice to override one property, I can revert back to the previous commit. |
Great catch! I don't think it's necessarily bad practice to override one property, it just comes down to preference. If we consider the alternative, we could remove the |
What's new in this PR
Description
Screenshots
//: # 'Required for frontend changes, otherwise optional but strongly recommended. Add screenshots of expected behavior - GIFs if you're feeling fancy! Use the provided image template. Drag the desired image into the PR, then copy the link into the placeholder.'
This is me adding 'Action Required' temporarily into the GreenStatusOptions enum and it updates accordingly.
How to review
Order of files changed works, main stuff is in utils and types file
Next steps
Need to make sure date conversion works when realtime dates are added via Retool and that whole configuration.
Relevant Links
Online sources
Related PRs
CC: @ronniebeggs