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

8693cb4dw create status button #10

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

Conversation

MaciejKubon
Copy link

Create status button
status-button

} from 'react-native';
import Svg, { Circle, Path } from 'react-native-svg';

type Param = {
Copy link
Contributor

Choose a reason for hiding this comment

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

type Props

onPress: (event: GestureResponderEvent) => void;
};

const StatusButton = (param: Param) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

param: Param => { title, status, onPress }: Props.

Dekstrukturyzacja obiektu

Comment on lines 89 to 110
const styles = StyleSheet.create({
button: {
justifyContent: 'center',
backgroundColor: '#ffffff',
width: 232,
height: 44,
borderRadius: 10,
paddingLeft: 10,
marginTop: 10,
shadowColor: '#0000001A',
shadowOffset: { width: 0, height: 1 },
shadowOpacity: 1,
shadowRadius: 6,
},
text: {
color: '#000000',
fontSize: 12,
paddingHorizontal: 30,
paddingVertical: 5,
position: 'absolute',
},
});
Copy link
Contributor

Choose a reason for hiding this comment

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

na pewno nie chcemy hard codować kolorów. Jeśli jakiś kolor nie jest zdefiniowany w pliku styles.ts, to zdefiniuj go, tak aby był dostępny globalnie. Pamiętaj o tym, że chcemy mieć jasny i ciemny motyw aplikacji, więc spraw, aby ten button był już gotowy w jednej i drugiej wersji

Comment on lines 11 to 15
type Param = {
title: string;
status: number;
onPress: (event: GestureResponderEvent) => void;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

ogólnie fajnie jakby button przyjmował wszystkie propsy jakie normalnie przyjmuje komponent <Pressable />

Zrób to w taki sposób:
type Props = React.ComponentPropsWithRef<typeof Pressable> & { jakieś propsy, których pressable nie przyjmuje, a potrzebujemy je w komponencie }

App.tsx Outdated
@@ -1,12 +1,19 @@
import { StatusBar } from 'expo-status-bar';
import { StyleSheet, Text, View } from 'react-native';
import { globalStyles } from 'style';

import StatusButton from './src/components/StatusButton';
Copy link
Contributor

Choose a reason for hiding this comment

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

używajmy aliasów: @/components

Copy link
Contributor

Choose a reason for hiding this comment

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

używajmy też plików reeksportujących. Zgodnie ze strukturą w readme. Plik reeksportujący to plik index.ts, w readme jest to index.tsx, ale zostanie to poprawione.

Oprócz tego możesz wrzucić ten komponent w folder /components/Button

Copy link
Contributor

Choose a reason for hiding this comment

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

mogę się dowiedzieć po co te ikony tutaj skoro one nie są nigdzie importowane?

};

const StatusButton = (param: Param) => {
const selectStatus = (property: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

ta funkcja wygląda słabo. Poszczególny case'y można wydzielić do osobnych komponentów


type Param = {
title: string;
status: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

nie podoba mi się to, że status jest typu number. Numer 0 nic tak naprawdę nie mówi o statusie. Docelowo to powinno być mapowane na jakiś union type

…ego + uzycie pliku reeksportujacego + przeniesienie przycisku + usniecie plikow svg + destrukturyzacja obiekotu
@MaciejKubon
Copy link
Author

Uzywanie aliasow + style globalne + poszczegolne casy w osobych komponentach + usuniecie zbednych ikon + zmian typu statusu + destrukutuyuzacja obiektu + użycie pliku reeksportujacego

Copy link
Contributor

@wiktorrudzki wiktorrudzki left a comment

Choose a reason for hiding this comment

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

Kilka rzeczy do poprawki

Copy link
Contributor

Choose a reason for hiding this comment

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

co to za plik

onPress: (event: GestureResponderEvent) => void;
};

const StatusButton = (param: Props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

param -> props

Copy link
Contributor

Choose a reason for hiding this comment

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

to niestety nie jest ani trochę hook, wrzuć to do komponentu

</View>
);
};
export default AccessibleStatusButton;
Copy link
Contributor

Choose a reason for hiding this comment

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

zachowuj odstępy pomiędzy takimi rzeczami jak komponent, export, style

borderRadius: 10,
paddingLeft: 10,
marginTop: 10,
shadowColor: '#000',
Copy link
Contributor

Choose a reason for hiding this comment

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

starajmy się nie używać hardcored colors tylko używajmy zmiennych z pliku style.cs. Co prawda nie jest on jeszcze uzupełniony zmiennymi zgodnymi z figmą, ale nie powielajmy złego podejścia

Copy link
Contributor

Choose a reason for hiding this comment

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

Jak coś to wszystko jest opisane w readme

const StatusButton = (param: Props) => {
const { themedStyles } = useTheme();
const actualStatus: Status = 'Inaccessible'; //added temporarily until the selected status is supported
const buttonStatus: Status = param.status;
Copy link
Contributor

Choose a reason for hiding this comment

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

nie musisz przypisywać tego do zmiennej jeśli i tak wykorzystujesz to w kodzie jeden raz


const StatusButton = (param: Props) => {
const { themedStyles } = useTheme();
const actualStatus: Status = 'Inaccessible'; //added temporarily until the selected status is supported
Copy link
Contributor

Choose a reason for hiding this comment

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

nie do końca rozumiem o co tutaj chodzi osobiście

Comment on lines +19 to +21
buttonBackground: 'blue',
buttonText: 'white',
buttonActive: '#E4E4E4',
Copy link
Contributor

Choose a reason for hiding this comment

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

nie używajmy kolorów typu blue, white, zamiast tego używajmy hexa zgodnie z figmą

Copy link
Contributor

Choose a reason for hiding this comment

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

Zauważ, że powielasz jeden schemat tworzenia komponentu 5 razy. Można to rozwiązać w tym przypadku następująco:
Ta część, która się powtarza może zostać wydzielona do osobnego komponentu, który przyjmuje props. Komponentem byłby text ze stylami, które byłyby zdefiniowane raz, w jednym miejscu, a do niego przekaż props text, który będzie inny w zależności od tego jaki to będzie status.

AccessibleStatusButton = () => {
.....
return {....... }
}

I to samo dla pozostałych, tylko przekażesz inny props text

Copy link
Contributor

Choose a reason for hiding this comment

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

daj znać po co jest jaka paczka, bo kojarzę, że coś tam było potrzebne, ale widzę teraz, że sporo tego wyszło

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.

2 participants