chore: update settings menu#1876
Conversation
ErikSin
left a comment
ErikSin
left a comment
There was a problem hiding this comment.
Good job on this. A couple small but blocking comments related to the SettingsRow component. The way it is designed leads to ambiguous behaviour and some unnecessary props that can be solved by making the code slightly more verbose, while still keeping the re usability of the component
There was a problem hiding this comment.
I would argue that this is not a screen. The screens have there own page in the Navigation/Stack/AppScreens or in Navigation/Tab. The drawer is only used as a side component that is shared between tabs.
There was a problem hiding this comment.
Lets rename this file and export to "Categories" to match the new terminology.
| function SectionHeader({label}: {label: string}) { | ||
| return ( | ||
| <BodyText variant="tinyMeta" style={styles.sectionHeader}> | ||
| {label} | ||
| </BodyText> | ||
| ); | ||
| } |
There was a problem hiding this comment.
This feels a little unnecessary as its just returning a BodyText. Lets get rid of the SectionHeader and just share the styles.sectionHeader
| testID={testID} | ||
| onPress={onPress} | ||
| background={TouchableNativeFeedback.Ripple(VERY_LIGHT_BLUE, false)}> | ||
| <View style={styles.row}> |
There was a problem hiding this comment.
Lets use a TouchableOpacity Instead. TouchableNativeFeedback is a remnant from mapeo, and is not compatible with iOS
| materialIconName?: MaterialIconsIconName; | ||
| icon?: React.ReactNode; |
There was a problem hiding this comment.
Instead of passing the MaterialIconName or Icon, lets just pass an icon. It will make the code a little more verbose as now we have to pass an entire react node, but it will get rid of the ambigous ternary code
| actionText?: string; | ||
| showArrow?: boolean; |
There was a problem hiding this comment.
Similar comment as the one related to the icon, this leads to some ambigous ternary code. IF there is both an actionText and a showArrow then only the action text will show up. Its leads to having unnecessary props that are not being used.
I think the better pattern here is to just have a prop that is a reactNode so that the developer can just directly pass in actionText or the Arrow icon
eg:
function SettingsRow{
label,
onPress,
EndContent
testID,
Icon,
}: {
label: string;
onPress: () => void;
EndContent:ReactNode
testID: string;
Icon: ReactNode
}){
return(
<TouchableOpacity>
{Icon}
<HeaderText> {label}</HeaderText>
{EndContent}
</TouchableOpacity>
)
}…y. Repeats code for section header. Passes react node for icon and end content.
2 participants
closes #1816
Note: In the Figma and Notion, there is project data in the Sharing Permissions section of the Comapeo Setting menu. According to a Slack message with Ximena, though, the content of the menu should not change, just the UI, so I did not add that project statistics sharing to the Comapeo Settings menu as it lives in Coordinator Tools.
Note: the
MenuListItemandFullScreenMenuare now only used in security since they are no longer used in the app settings/index. These probably don't need to be shareable components any more. However, there are so many diffs in this PR I thought it would make more sense to clean that up in a separate ticket/ pr.Question: There are spanish translations in the Notion. Should I be adding those in Crowdin?
Screenshots
Regular Size
Large Size
