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

fix: Enhance SelectInput and DAOSelector #669

Closed
wants to merge 1 commit into from
Closed

Conversation

WaDadidou
Copy link
Collaborator

@WaDadidou WaDadidou commented Aug 15, 2023

  • Use SelectInput in DAOSelector.
  • Enhance SelectInput and add the possibility to enters a value to search in the SelectInput list (Not effective for DAOSelector, the image is just an example)

image

image

Before, we used @react-native-picker/picker
image

See

export const DAOSelector: React.FC<{

And
export const SelectInput: React.FC<Props> = ({

@netlify
Copy link

netlify bot commented Aug 15, 2023

Deploy Preview for teritori-dapp ready!

Name Link
🔨 Latest commit 255dfac
🔍 Latest deploy log https://app.netlify.com/sites/teritori-dapp/deploys/64de7f8a1d0a84000894ace6
😎 Deploy Preview https://deploy-preview-669--teritori-dapp.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Aug 15, 2023

Deploy Preview for testitori ready!

Name Link
🔨 Latest commit 255dfac
🔍 Latest deploy log https://app.netlify.com/sites/testitori/deploys/64de7f8a7400f70008755253
😎 Deploy Preview https://deploy-preview-669--testitori.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@WaDadidou WaDadidou force-pushed the fix/DAOSelector branch 2 times, most recently from 3a8bfae to 3bd6191 Compare August 15, 2023 13:58
omniwired
omniwired previously approved these changes Aug 16, 2023
@WaDadidou WaDadidou force-pushed the fix/DAOSelector branch 2 times, most recently from 2bf7627 to 9e2a1b0 Compare August 17, 2023 19:43
Copy link
Collaborator

@n0izn0iz n0izn0iz left a comment

Choose a reason for hiding this comment

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

did not read everything yet

Comment on lines +96 to +98
export const useNSPrimaryAliases = (userIds: string[] | undefined) => {
const { data, ...other } = useQuery(
nsPrimaryAliasQueryKey(userIds?.map((id) => id.slice(-3)).toString()),
Copy link
Collaborator

@n0izn0iz n0izn0iz Aug 17, 2023

Choose a reason for hiding this comment

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

this breaks caching, please use useNSPrimaryAlias deeper in the component tree

Comment on lines +293 to +295
inputIconContainer: {
marginLeft: layout.padding_x1_5,
},
Copy link
Collaborator

@n0izn0iz n0izn0iz Aug 17, 2023

Choose a reason for hiding this comment

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

please don't use stylesheet.Create or add new fields in stylesheet.Create, this creates potential undetectable deadcode and has no performance impact contrary to popular belief

if you want to define constant styles to prevent re-renders on memoized components, use something like

export const SomeComponent: React.FC = () => {
   return <View style={inputIconContainerStyle} />
}

const inputIconContainerStyle: ViewStyle = {
  marginLeft: layout.padding_x1_5,
}

@@ -71,7 +71,6 @@ export const TNSManageScreen: React.FC<TNSManageScreenProps> = ({
</BrandText>

<DAOSelector
value={selectedDAOId}
Copy link
Collaborator

Choose a reason for hiding this comment

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

are you sure about this change? uncontrolled inputs can be a pain

@WaDadidou WaDadidou closed this Aug 18, 2023
@WaDadidou WaDadidou deleted the fix/DAOSelector branch August 18, 2023 21:04
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.

3 participants