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

#322 Add PeriodicityModalScreen #341

Merged
merged 3 commits into from
Oct 22, 2021

Conversation

BatDroid
Copy link
Contributor

✅ I have read the contributing file

Summary

Adding screen as explained in #322

Changelog

Adding Periodicity screen which can be opened by calling navigator.openPeriodicityModal();.
(the button which leads the user to it is not added yet, I'm willing to work on that issue).

Demo

Simulator Screen Shot - iPhone 11 - 2021-10-20 at 02 14 12
Simulator Screen Shot - iPhone 11 - 2021-10-20 at 02 13 56

Copy link
Member

@PierreBresson PierreBresson left a comment

Choose a reason for hiding this comment

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

Clean PR, just few changes and we are good to go 👍

@@ -9,12 +9,14 @@ type Props = {
text: string;
isSelected: boolean;
onPress: () => void;
touchableStyle?: StyleProp<ViewStyle>;
Copy link
Member

Choose a reason for hiding this comment

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

Please let's keep shared component style within the component as much as possible. If you want to add margin, you can add it inClickableTag.style.tsx - so it's easier to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I thought that maybe it's better not to modify other codes

@@ -30,6 +31,11 @@ const AddEmissionNavigator = (): React.ReactElement => (
options={BarCodeScanScreen.navigationOptions}
component={BarCodeScanScreen}
/>
<Stack.Screen
Copy link
Member

Choose a reason for hiding this comment

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

Should be place inside ModalNavigator because right now it's not opening as a modal

@@ -23,6 +23,10 @@ const openInfoModal = (navigation) => (props = {}) => {
});
};

const openPeriodicityModal = (navigation) => (props = {}) => {
navigation.push("PeriodicityModal", props);
Copy link
Member

Choose a reason for hiding this comment

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

Not tested, but I can guess something like this would work :

const openPeriodicityModal = (navigation) => (props = {}) => {
  navigation.navigate("PeriodicityModal", {
    screen: "PeriodicityModal",
    params: props,
  });
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

having a function like this worked with the way I implemented it. adding it to ModalNavigator made it work strangely.(you can test it), besides I would have needed to call it like:

const openPeriodicityModal = (navigation) => (props = {}) => {
  navigation.navigate("CommingSoon", { // because, coming soon is the first screen on navigator
    screen: "PeriodicityModal",
    params: props,
  });
};

</View>
)}
<View style={styles.tagSection}>
<Text.H3>{t("PERIODICITY_MODAL_SCREEN_OCCURENES")}</Text.H3>
Copy link
Member

Choose a reason for hiding this comment

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

PERIODICITY_MODAL_SCREEN_OCCURRENCES

onPress={() => {
setTimes(index + 1);
}}
text={`${index + 1} ${t(`PERIODICITY_MODAL_SCREEN_TIME${index > 0 ? "S" : ""}`)}`}
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately not all languages have the same system for plurial (1 gang/2 gange in danish) so maybe this would work better :
${index > 0 ? t("PERIODICITY_MODAL_SCREEN_TIME"): t("PERIODICITY_MODAL_SCREEN_TIMES")}

dayIndex: number;
nameKey: keyof TranslationKeys;
}[] = [
{ dayIndex: 2, nameKey: "MONDAY" },
Copy link
Member

Choose a reason for hiding this comment

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

Since the days will be use later on, you can put them in app/utils/ui/translations/en.json and use them like this : UI_MONDAY

@@ -0,0 +1,3 @@
{
"INFO_MODAL_SCREEN_METHODOLOGY": "Metodología"
Copy link
Member

Choose a reason for hiding this comment

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

{}

@@ -0,0 +1,3 @@
{
"INFO_MODAL_SCREEN_METHODOLOGY": "Methodologie"
Copy link
Member

Choose a reason for hiding this comment

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

{}

@@ -0,0 +1,3 @@
{
"INFO_MODAL_SCREEN_METHODOLOGY": "Metodologi"
Copy link
Member

Choose a reason for hiding this comment

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

{}

@@ -0,0 +1,3 @@
{
"INFO_MODAL_SCREEN_METHODOLOGY": "方法"
Copy link
Member

Choose a reason for hiding this comment

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

{}

@BatDroid
Copy link
Contributor Author

hope changes are acceptable & I can start working on other issues.
PS: Please send me stickers for this year (I regret not accepting last year 😄 )

Copy link
Member

@PierreBresson PierreBresson left a comment

Choose a reason for hiding this comment

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

Perfect ✌️

@PierreBresson
Copy link
Member

PierreBresson commented Oct 22, 2021

@BatDroid yes, no problem, just email me your address and I will send you a bunch of stickers 🙂
Thanks!

@PierreBresson PierreBresson merged commit 9c9948e into NMF-earth:main Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants