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

added toast progress #61

Open
wants to merge 6 commits into
base: development
Choose a base branch
from

Conversation

DanutIlie
Copy link

Feature
Added toast progress tracking and toast lifetime

Contains breaking changes

[x] No

[] Yes

Updated CHANGELOG

[x] Yes

Testing

[x] User testing
[] Unit tests

@@ -71,6 +71,7 @@
"@swc/jest": "^0.2.36",
"@types/jest": "29.5.13",
"@types/lodash": "4.17.4",
"@types/lodash.isequal": "4.5.8",
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove lodash dependency and add depenency for lodash.isstring, lodash.isequal

Copy link
Author

Choose a reason for hiding this comment

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

done


export class ToastManager {
private transactionToastsList: TransactionToastList | undefined;
private toastProgressManager: ToastProgressManager;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ProgressManager and LifetimeManager

Copy link
Author

Choose a reason for hiding this comment

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

done

if (!this.transactionToastsList) {
this.transactionToastsList =
private handleProgressUpdate = (toastId: string, progress: number) => {
const toastIndex = this.transactionToasts.findIndex(
Copy link
Collaborator

Choose a reason for hiding this comment

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

use Map ?

Copy link
Author

Choose a reason for hiding this comment

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

in this case is not fit Map usually Map is use to map object to value is very similar with Record<type,type> but it can support also functions

import { getAddressFromDataField } from 'utils';
import { isCrossShardTransaction } from './isCrossShardTransaction';

export const getAreTransactionsOnSameShard = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

add basic test?

Copy link
Author

Choose a reason for hiding this comment

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

done


const invalidToastData: IToastDataState = {
id: toastId,
icon: 'ban',
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider creating an enum for icons so we can replace them easyer in the future

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -65,7 +65,9 @@ export async function initApp({
}

trackTransactions();
const toastManager = new ToastManager();
const toastManager = new ToastManager({
successfulToastLifetime: 10000
Copy link
Collaborator

Choose a reason for hiding this comment

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

check if this is currenlty the same as today and maybe define it in constants folder

Copy link
Author

Choose a reason for hiding this comment

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

i add it to dapConfig because it doesn't have a default, if they are not provided in the dapp it will not expire

const zeroAddress = Buffer.alloc(32).fill(0);
return pubKey.equals(zeroAddress);
};
export const getShardOfAddress = (hexPubKey: any) => {

Choose a reason for hiding this comment

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

can it be typed instead of any ?

Copy link
Author

Choose a reason for hiding this comment

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

it cannot without rewriting the function


switch (status) {
case TransactionBatchStatusesEnum.signed:
case TransactionBatchStatusesEnum.sent:

Choose a reason for hiding this comment

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

63,64 can be put last

case TransactionBatchStatusesEnum.signed:
case TransactionBatchStatusesEnum.sent:
default:
return pendingToastData;

or simply remove them and it will be on default

Copy link
Author

Choose a reason for hiding this comment

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

done

}

export class LifetimeManager {
// eslint-disable-next-line no-undef

Choose a reason for hiding this comment

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

Why this comment?

Copy link
Author

Choose a reason for hiding this comment

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

for some reason(don't know yet why) it doesn't recognise NodeJS

}

export class ProgressManager {
// eslint-disable-next-line no-undef

Choose a reason for hiding this comment

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

Why comment?

Copy link
Author

Choose a reason for hiding this comment

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

for some reason(don't know yet why) it doesn't recognise NodeJS


return transactions.reduce(
(prevTxIsSameShard: boolean, { receiver, data }: SignedTransactionType) => {
const receiverAddress = getAddressFromDataField({

Choose a reason for hiding this comment

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

Why get from data field?

Copy link
Author

Choose a reason for hiding this comment

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

it's from old sdk-dapp, it was taken from there

} catch (_err) {
return -1;
}
};

Choose a reason for hiding this comment

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

Unit tests

Copy link
Author

Choose a reason for hiding this comment

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

done

}

return `${processedTransactions} / ${totalTransactions} transactions processed`;
};

Choose a reason for hiding this comment

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

Tests

Copy link
Author

Choose a reason for hiding this comment

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

done

} catch (_err) {
return false;
}
}

Choose a reason for hiding this comment

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

Tests

Copy link
Author

Choose a reason for hiding this comment

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

done

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.

4 participants