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: tokenData are passed now from AddTokenByNetwork consistent with the chosen network #1507

Merged

Conversation

AntonioVentilii-DFINITY
Copy link
Contributor

Motivation

To be more consistent with the selected network from the dropdown, the data returned are set when the component is destroyed.

Changes

Prop tokenData is defined with onDestroy.

Copy link
Member

@peterpeterparker peterpeterparker left a comment

Choose a reason for hiding this comment

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

LGTM, thx

erc20ContractAddress
};
onDestroy(() => {
tokenData = isNetworkIdICP(network?.id)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment about why we do this as it's quite an original pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check now, I updated the logic, maybe it is better?

@AntonioVentilii-DFINITY
Copy link
Contributor Author

@peterpeterparker check the new version, maybe it is better?

p.s. I tried to do inline,

$: tokenData = isNetworkIdICP(network?.id)
		? { ledgerCanisterId, indexCanisterId }
		: isNetworkIdEthereum(network?.id)
			? { erc20ContractAddress }
			: {};		

but it was giving an error because it was interpreting this as assigning undefined keys

@AntonioVentilii-DFINITY AntonioVentilii-DFINITY force-pushed the bugfix/token-data-from-add-token-are-consistent branch from 9148054 to ae267e8 Compare June 21, 2024 09:07
Copy link
Member

@peterpeterparker peterpeterparker left a comment

Choose a reason for hiding this comment

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

I like it better!

Thanks

@AntonioVentilii-DFINITY AntonioVentilii-DFINITY merged commit 6319a11 into main Jun 21, 2024
6 checks passed
@AntonioVentilii-DFINITY AntonioVentilii-DFINITY deleted the bugfix/token-data-from-add-token-are-consistent branch June 21, 2024 09:53
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