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.

Sign up for GitHub

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

[TS migration] Migrate 'SelectCircle.js' component to TypeScript #30077

Merged
merged 9 commits into from
Nov 6, 2023
Merged

[TS migration] Migrate 'SelectCircle.js' component to TypeScript #30077

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
56172e9
Migrate to TS
MaciejSWM Oct 20, 2023
dcb7386
Restore default values
MaciejSWM Oct 20, 2023
0b620d0
Use StyleProp
MaciejSWM Oct 23, 2023
f8b0614
Make styles optional
MaciejSWM Oct 25, 2023
8196502
Optional styles
MaciejSWM Oct 25, 2023
d64538a
Merge branch 'ts-migration/SelectCircle' of https://github.com/softwa…
MaciejSWM Oct 25, 2023
fd36698
Drop default value - undefined works too
MaciejSWM Oct 26, 2023
ba7baf6
Merge branch 'main' into ts-migration/SelectCircle
MaciejSWM Oct 31, 2023
6535cea
Reorder imports - eslint
MaciejSWM Nov 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 0 additions & 40 deletions src/components/SelectCircle.js
View file
Open in desktop

This file was deleted.

31 changes: 31 additions & 0 deletions src/components/SelectCircle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import React from 'react';
import {StyleProp, View, ViewStyle} from 'react-native';
import globalStyles from '../styles/styles';
import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import themeColors from '../styles/themes/default';

type SelectCircleProps = {
/** Should we show the checkmark inside the circle */
isChecked: boolean;

/** Additional styles to pass to SelectCircle */
styles?: StyleProp<ViewStyle>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no way to define a default for this prop like []?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is possible, we can default it to []. cc @MaciejSWM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @cead22. Thanks for spotting this. It turns out that no action is needed, as RN+TS still work well when styles are undefined. Please see this:
https://github.com/Expensify/App/pull/29988/files#r1373207252
Especially the first example that passes no styles to FullScreenLoadingIndicator: https://github.com/Expensify/App/pull/29988/files#r1374544242

};

function SelectCircle({isChecked = false, styles}: SelectCircleProps) {
return (
<View style={[globalStyles.selectCircle, globalStyles.alignSelfCenter, styles]}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typescript made me get rid of the spread operator (...styles -> styles) after I switched from an Array<> to StyleProp<>. But the weird thing is that it still works when styles are passed like they used to be: <SelectCircle isChecked={props.isSelected} styles={[styles.singleOptionSelectorCircle]} />

Copy link
Contributor

Choose a reason for hiding this comment

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

If it works, that's fine with me. Please test that the styles are displayed correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, works just fine

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check that styles isn't null?

Copy link
Contributor

@blazejkustra blazejkustra Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@cead22 Styles accept falsy values, so no need for such check:

type Falsy = undefined | null | false;
export type StyleProp<T> =
  | T
  | RegisteredStyle<T>
  | RecursiveArray<T | RegisteredStyle<T> | Falsy>
  | Falsy;

{isChecked && (
<Icon
src={Expensicons.Checkmark}
fill={themeColors.iconSuccessFill}
/>
)}
</View>
);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We should always define displayName:

Suggested change
SelectCircle.displayName = 'SelectCircle';

SelectCircle.displayName = 'SelectCircle';

export default SelectCircle;