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

CU-86dt1fxtx-1 - Implement Contextual Messages on WcSdk on both sides #116

Merged
merged 1 commit into from
May 15, 2024

Conversation

LeonardoDizConde
Copy link
Contributor

No description provided.

@melanke
Copy link
Contributor

melanke commented May 14, 2024

@@ -32,7 +33,7 @@ function HelloWorld() {

const getMyBalance = async (): Promise<void> => {
try {
const resp = await wcSdk.testInvoke({
const resp = await wcSdk.withContext(context).testInvoke({
Copy link
Contributor

Choose a reason for hiding this comment

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

You've replaced all the calls to use withContext but I think we shouldn't do this.
This makes it look like the context message is mandatory, and since it's optional, it will not be used always. Let's simply create a new test method that contains a hardcoded context message to be used as example.

Comment on lines 536 to 543
Dapp method invocation context (optional):
<br></br>
<textarea
data-testid="hello-world__contextual_message"
placeholder="Write a context message for method invocation"
onChange={(event) => setContext(event.target.value)}
/>
<br></br>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this, let's keep the examples simple, let's just use an hardcoded context message

@@ -79,6 +80,7 @@ export const DEFAULT_AUTO_ACCEPT_METHODS: Method[] = [
'getNetworkVersion',
'calculateFee',
'wipeRequests',
//'withContext'
Copy link
Contributor

Choose a reason for hiding this comment

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

delete this line

@LeonardoDizConde LeonardoDizConde force-pushed the CU-86dt1fxtx-1 branch 2 times, most recently from 5f5d4a0 to b932014 Compare May 15, 2024 15:37
@@ -59,6 +61,12 @@ export default function RequestCard(
Neo3
</Text>
</Flex>
<Text fontSize="0.875rem" color="#888888" fontWeight="bold" mt="0.875rem">

Choose a reason for hiding this comment

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

Ensure that the color #888888 is consistent with the theme or color scheme of the application. If there is a theme file or constants for colors, consider using that instead of hardcoding the color value.

Method Contextual Message
</Text>
<Text fontSize="0.875rem" mt="0.5rem" data-testid="request-card__contextual_message">
{contextualMessage}

Choose a reason for hiding this comment

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

The data-testid attribute value request-card__contextual_message should follow a consistent naming convention. Verify that it aligns with other data-testid values throughout the project for consistency.

await connectReactDappToNewReactAccount(context, dappPage, walletPage)
await dappPage.awaitAndClickTestId('hello-world__verify-with-context')
await acceptPendingRequestToReactWallet(walletPage, async (walletPageBeforeAcceptMethod) => {
const contextMessage = await walletPageBeforeAcceptMethod.awaitAndGetTestId('request-card__contextual-message')

Choose a reason for hiding this comment

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

Ensure that the test ID request-card__contextual-message is consistently used across all relevant test files to avoid potential discrepancies in element targeting.

@@ -31,6 +31,8 @@ export default function RequestCard(
props.closeRequest()
}

const contextualMessage = String(request.params.contextualMessage).trim()

Choose a reason for hiding this comment

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

Ensure that request.params.contextualMessage exists before converting it to a string and trimming it. This could potentially throw an error if contextualMessage is undefined or not present in request.params.

@melanke melanke merged commit 3686bd3 into main May 15, 2024
2 checks passed
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