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

1137: Adding new intro slides #3014

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

Conversation

bahaaTuffaha
Copy link
Contributor

@bahaaTuffaha bahaaTuffaha commented Dec 2, 2024

Short description

Changing and adding new intro slides to look like this here

Proposed changes

  • Added textStyle prop to TextButton to let me change text styling.
  • Because the new images are common across all apps I added them at a new folder : build-configs/common/assets
  • The colors of these new images will change dynamically depending on the theme.(using currentColor)
  • I didn't remove the old images.

Note: not tested on iOS.

Side effects

  • components/Pagination.tsx
  • components/TextButton.tsx

Testing

  • Just clear the app's data and open the app.
  • The intros should appear ✨.

Fixes: #1137


Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

I love the design, just a few minor comments :) Thank you!
One more thing: Please delete the old assets as they are not needed anymore.

@@ -42,15 +41,13 @@ type SlideContentProps = {
const SlideContent = ({ item, width }: SlideContentProps): ReactElement => (
<ScrollView
contentContainerStyle={{
flexGrow: 1,
height: '100%',
Copy link
Member

Choose a reason for hiding this comment

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

🤔

@@ -3,6 +3,7 @@ import { useTranslation } from 'react-i18next'
import { FlatList, useWindowDimensions, ViewToken } from 'react-native'
import styled, { css } from 'styled-components/native'

import welcome from 'build-configs/common/assets/welcome.svg'
Copy link
Member

Choose a reason for hiding this comment

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

🔧 Please don't import assets directly. Instead, add them to build config

@@ -18,41 +19,49 @@ const Container = styled.View<{ width: number }>`
flex: 1;
flex-direction: column;
width: ${props => props.width}px;
justify-content: space-between;
padding-bottom: 30%;
background-color: white;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
background-color: white;
background-color: ${props => props.theme.colors.backgroundColor};

Comment on lines +16 to +18
/* flex: 1; */
width: 139px;
height: 40px;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not set a fixed width and instead approximate it with the corresponding padding, e.g.

Suggested change
/* flex: 1; */
width: 139px;
height: 40px;
padding: 12px 32px;

Copy link
Contributor Author

@bahaaTuffaha bahaaTuffaha Dec 5, 2024

Choose a reason for hiding this comment

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

This is StyledButton ...using padding for the button? 🤔

Search: styled(icons.Search)`
${ImageStyle};
`,
Events: styled(icons.Events)`
Directions: styled(icons.directions)`
Copy link
Member

Choose a reason for hiding this comment

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

🔧 Please stick to our internal naming pois (same for all other occurrences)

Suggested change
Directions: styled(icons.directions)`
Pois: styled(icons.pois)`

Furthermore, please only show this slide if pois is enabled in the feature flags in the build config.

platforms: # relevant platforms, possible values: web, android and ios
- android
- ios
en: Added new intro screens.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
en: Added new intro screens.
en: Redesigned our app intro.
de: Neues Design für unsere App-Einführung.

Comment on lines +41 to +45
directions: directionsIcon,
Language: languageIcon,
Search: searchIcon,
Offline: offlineIcon,
information: informationIcon,
Copy link
Member

Choose a reason for hiding this comment

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

🔧 Please keep the naming consistent here and user upper case names

Language: integreatIntroLanguageIcon,
Search: integreatIntroSearchIcon,
},
intro: commonIntros,
Copy link
Member

Choose a reason for hiding this comment

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

Very nice that we can just use this for every build config, nice implementation! 🎉 We could even think of implementing this for aschaffenburg easily 🤔 I'll get back to you in regards to that.

border-radius: 5px;
margin: 0 4px;
margin: 0 18px;
Copy link
Member

Choose a reason for hiding this comment

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

💭 Perhaps we should increase the touch area that responds to press, I find it quite hard at the moment to actually press a dot and go to a specific slide

width: 10px;
height: 10px;
width: 12px;
height: 12px;
border-radius: 5px;
Copy link
Member

Choose a reason for hiding this comment

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

🔧 Please adjust the radius as well, otherwise the dots are not circles as shown in the design

Suggested change
border-radius: 5px;
border-radius: 6px;

Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Sorry, I approved by accident

"offline": "Funktioniert auch offline",
"offlineDescription": "Du hast nicht immer Internet? Kein Problem. Du kannst Integreat auch ohne Internet benutzen.",
"information": "Aktuelle Informationen",
"informationDescription": "Was gibt es Neues in deiner Nähe? 
Finde es heraus in Integreat.",
Copy link
Member

Choose a reason for hiding this comment

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

🔧 There was a mean LSEP hidden in the translation:

Suggested change
"informationDescription": "Was gibt es Neues in deiner Nähe? Finde es heraus in Integreat.",
"informationDescription": "Was gibt es Neues in deiner Nähe? Finde es heraus in Integreat.",

"languageChange": "Sprachwechsel",
"languageChangeDescription": "Einfaches Umstellen der Sprache einzelner Seiten",
"welcome": "Willkommen bei {{appName}}",
"appDescription": "In {{appName}} findest du Antworten und Tipps in deiner Muttersprache. Zum Beispiel: Wo finde ich Hilfe? Wann darf ich arbeiten?",
Copy link
Member

Choose a reason for hiding this comment

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

🔧 Please adjust the naming

Suggested change
"appDescription": "In {{appName}} findest du Antworten und Tipps in deiner Muttersprache. Zum Beispiel: Wo finde ich Hilfe? Wann darf ich arbeiten?",
"welcomeDescription": "In {{appName}} findest du Antworten und Tipps in deiner Muttersprache. Zum Beispiel: Wo finde ich Hilfe? Wann darf ich arbeiten?",

"offline": "Funktioniert auch offline",
"offlineDescription": "Du hast nicht immer Internet? Kein Problem. Du kannst Integreat auch ohne Internet benutzen.",
"information": "Aktuelle Informationen",
"informationDescription": "Was gibt es Neues in deiner Nähe? 
Finde es heraus in Integreat.",
"skip": "Überspringen",
"next": "Weiter"
},
"am": {
"appDescription": "{{appName}} ለጀርመን የእርስዎ የዲጂታል መሪ ነው",
Copy link
Member

Choose a reason for hiding this comment

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

🔧 The keys appDescription, events and eventsDescription should also be deleted as the German translation changed/they are not used anymore.

"directions": "Angebote in deiner Nähe",
"directionsDescription": "In der Karte kannst du Orte in deiner Nähe finden, zum Beispiel Orte für Sprachkurse.",
"offline": "Funktioniert auch offline",
"offlineDescription": "Du hast nicht immer Internet? Kein Problem. Du kannst Integreat auch ohne Internet benutzen.",
Copy link
Member

Choose a reason for hiding this comment

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

🔧 Please replace all occurrences of Integreat with {{appName}} and use the translations accordingly.

"search": "Αναζήτηση",
"searchDescription": "Βρείτε γρήγορα τις σωστές πληροφορίες χρησιμοποιώντας λέξεις-κλειδιά",
"languageChange": "Αλλαγή γλώσσας",
"languageChangeDescription": "Αλλάξτε εύκολα τη γλώσσα μεμονωμένων σελίδων",
"events": "Εκδηλώσεις",
"eventsDescription": "Τρέχουσες εκδηλώσεις και ημερομηνίες στην περιοχή",
"skip": "Παραλείπω",
"next": "Συνέχεια"
},
"en": {
Copy link
Member

Choose a reason for hiding this comment

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

❌ Please make sure to add english translations as well.

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.

Add intro slides for news and pois and redesign existing
2 participants