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

Implement Dashboard Activity #131

Merged
merged 35 commits into from May 24, 2023
Merged

Implement Dashboard Activity #131

merged 35 commits into from May 24, 2023

Conversation

JSKitty
Copy link
Member

@JSKitty JSKitty commented May 14, 2023

Abstract

This PR adds the well-needed Dashboard Activity; which is a full overview of all of your wallet's historical transactions, the Activity area displays a generative description/icon/colour for the context of the Tx, and is designed flexibly to easily add/edit/remove Tx types.

image

To optimise this with the old 'Staking List', I've reworked the 'arrRewards' array to instead hold ALL historical transactions in a slimmed-down data format, implementing type filtering, so now all Activity lists can use the same array source, as well.

The PR is mostly complete functionally, there's only some small bits remaining, such as:

  • Routine loading of new on-chain transactions during session
  • Render all Activity tables simultaneously, for better UX
  • Improving Mobile design (some minor CSS bugs)
  • Consider verbose descriptions (with destinations, senders, etc)

What does this PR address?

It gives the user easy access to their full historical wallet information, every transaction ever made, in an easy and digestible format.

What features or improvements were added?

The Dashboard's 'Activity' list, as well as a redesign of the Activity renderer in general for better flexibility and re-usability across various MPW features.

How does this benefit users?

Information! Lots of it, folks love to see their wallet history.

@JSKitty JSKitty added the Enhancement New feature or request label May 14, 2023
@JSKitty JSKitty self-assigned this May 14, 2023
@JSKitty JSKitty requested review from Liquid369, Duddino, BreadJS and a team May 19, 2023 16:20
Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

Left a couple of commets, overall really like the PR

scripts/network.js Outdated Show resolved Hide resolved
scripts/network.js Outdated Show resolved Hide resolved
* @param {number} blockHeight - The block height of the transaction.
* @param {number} amount - The amount transacted, in coins.
*/
constructor(
Copy link
Member

Choose a reason for hiding this comment

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

This should probably take a destructured object

Copy link
Member Author

Choose a reason for hiding this comment

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

A bit of a 'nit of a nit' on my behalf, but, I think this won't be greatly useful, for reasons:

  • It requires double 'typing' the parameters (of the object to deconstruct) and then the internal properties of the Class itself, wastes a bit of space in our files.

  • Even with typing the object argument, and then it's properties, VS Code then will not be able to properly auto-fill the types of the Object, in the rare case that a historical TX must be manually constructed (perhaps for the Mempool in the future), see image:
    image

  • Frequent 'constructing/deconstructing' of objects does slightly add pressure to the GC, inflates memory usage, and is generally slower when done en-masse, especially when there'll be thousands of these entries for large wallets. (Not huge enough to TRULY hurt anything, but just something nitty to keep in mind).

All that said... it won't nuke MPW, so if you still prefer it, I'll throw it in.

Copy link
Member

Choose a reason for hiding this comment

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

It requires double 'typing' the parameters (of the object to deconstruct) and then the internal properties of the Class itself, wastes a bit of space in our files.

My editor isn't properly typing the parameters unless I declare the fields manually (No jsdoc required), for example:

class A {
a;
/**
* @param{string} a
*/
constructor(a) {
    this.a = a;
}
}

Makes this.a a string. If i remove the a; declaration, it becomes any. This is regardless of how the parameters were passed

Even with typing the object argument, and then it's properties, VS Code then will not be able to properly auto-fill the types of the Object, in the rare case that a historical TX must be manually constructed (perhaps for the Mempool in the future), see image:

Can you send another image demonstrating what it's supposed to do?

Frequent 'constructing/deconstructing' of objects does slightly add pressure to the GC, inflates memory usage, and is generally slower when done en-masse, especially when there'll be thousands of these entries for large wallets. (Not huge enough to TRULY hurt anything, but just something nitty to keep in mind).

Surely the modern js engines will optimize this? I'm just guessing, but I doubt it would be impactful

Either way, I think it's fine leaving it like this, because it's only being constructed once

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes this.a a string. If i remove the a; declaration, it becomes any. This is regardless of how the parameters were passed.

Ah, I see what you mean now - Indeed the class remains dependent on manual typing regardless of the param method used.

Can you send another image demonstrating what it's supposed to do?

VSC is correctly prompting the current param, during construction (and in the case there's hardcoded 'test'|'test2'|'test3' types it will also suggest these automatically).
image

Surely the modern js engines will optimize this? I'm just guessing, but I doubt it would be impactful.

I had tested this previously with NobleJS back in the Ancient Days:tm: of MPW, I was tracking down excessive GCs and memory clutter for speeding up VanityGen and was reading deep in to V8 optimisation, on tests discovered that the allocations were from filling the GC with dead objects (each de/structure is a new variable or set-of variables, after all, it can't re-use prior allocated objects), the quirks of JS!

scripts/network.js Show resolved Hide resolved
scripts/network.js Outdated Show resolved Hide resolved
scripts/network.js Outdated Show resolved Hide resolved
scripts/global.js Outdated Show resolved Hide resolved
scripts/global.js Outdated Show resolved Hide resolved
JSKitty and others added 9 commits May 22, 2023 16:03
Co-authored-by: Duddino <duddino@duddino.com>
Co-authored-by: Duddino <duddino@duddino.com>
Co-authored-by: Duddino <duddino@duddino.com>
Co-authored-by: Duddino <duddino@duddino.com>
Co-authored-by: Duddino <duddino@duddino.com>
@JSKitty JSKitty requested a review from Duddino May 23, 2023 13:54
Copy link
Member

@Duddino Duddino left a comment

Choose a reason for hiding this comment

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

tACK, works really well this is another milestone in MPW development

Copy link
Member

@SKDTXL SKDTXL left a comment

Choose a reason for hiding this comment

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

tACK, works perfectly well. The activity dashboard looks pretty clean, and in good order.

Copy link

@Liquid369 Liquid369 left a comment

Choose a reason for hiding this comment

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

tACK 50808e1
Much faster now, way better than originally its working very well with larger wallets LGTM

@JSKitty JSKitty merged commit f84fb1b into master May 24, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants