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

Rewrite activity in vue #158

Merged
merged 31 commits into from Sep 21, 2023
Merged

Rewrite activity in vue #158

merged 31 commits into from Sep 21, 2023

Conversation

Duddino
Copy link
Member

@Duddino Duddino commented Jul 19, 2023

Abstract

This is the first of a hopefully series of PRs aimed at rewriting parts MPW in Vue.js

Why vue.js?

Vue is a fairly light framework, and can be integrated into our current codebase from the botton up, meaning that we can start by rewriting singular components, then entire tabs and maybe the entire page eventually.
The syntax is very similar to old school web development, but it avoids a lot of the boilerplate.
For instance, take this snippet of code, repeated many times in our codebase:

document.getElementById('id').style.display = variable ? 'block' : 'none';

In vue, this becomes

<div v-if="variable"> </div>

Note that we no longer need to assign an id to the div.
Other benefits include:

  • The ability to split code in multiple files (index.template.html no longer needs to include everything).
  • Harder to include vulnerabilities such as Xss
  • Ability to containerize css (and js) to the component, and avoiding having big namespaces
  • Probably more?

Why start with the activity?

The activity list is a fairly complex component, which is reused two times (Dashboard and stake tab). It is a perfect example on how other parts can be rewritten as it needs to fetch transaction from the network, process the information and display it.


Testing

  • Test that the activity and stake reward list matches with app.mypivxwallet.org.
  • Check that both of the lists are changed upon translation.
  • Check that both of the lists update properly when Translations are changed.

@JSKitty JSKitty self-requested a review July 21, 2023 10:39
@JSKitty JSKitty added the Enhancement New feature or request label Jul 21, 2023
JSKitty
JSKitty previously approved these changes Jul 21, 2023
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

I've read this implementation over a few times now, and tested lightly as well.

Honestly: I think this is a great idea, it looks plenty flexible and easy enough for us to learn and adapt to, it chips away at our huge global.js mess, and it reduces the HTML template size a ton, this is definitely the modularisation we needed, in my opinion.

Onwards to Vue.js, I say...

A bug note: the Staking Activity exclusively is not loading for me, and when attempting to load it, my Firefox is freezing up - no errors logged, maybe a bad loop:
image

@Duddino
Copy link
Member Author

Duddino commented Jul 25, 2023

A bug note: the Staking Activity exclusively is not loading for me, and when attempting to load it, my Firefox is freezing up - no errors logged, maybe a bad loop:

That was because I forgot to properly pass fNewOnly, should be fixe now

@Duddino Duddino marked this pull request as ready for review July 25, 2023 12:20
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.

Overall it's looking good, one nit if we really need that console.log I commented on.
It runs very well for my large testnet wallet, loading quickly and not freezing up on me gg

scripts/Activity.vue Outdated Show resolved Hide resolved
Liquid369
Liquid369 previously approved these changes Jul 26, 2023
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 8393b78

I am excited about the Vue work! GG @Duddino

Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

There's one minor regression compared to app.mypivxwallet.org: After loading the Activity on the Dashboard automatically, the 'Stake History' is not rendered, so when the user navigates to the Staking tab, it will always be empty unless you manually hit "Load More" and sync additional history data (more than necessary) from the network.

Previously, MPW renders both at the same time, to prevent this edgecase.

Additional note: Since the merging of #160, this PR needs updating with Translation tags for all Activity UI text.

scripts/Activity.vue Outdated Show resolved Hide resolved
@JSKitty JSKitty added Awaiting Review This PR and/or issue is awaiting reviews before continuing. Review Reward: 10 PIV Reviewers of this Pull Request will receive a 10 PIV reward labels Sep 19, 2023
panleone
panleone previously approved these changes Sep 19, 2023
Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

tACK 2eec65e
I really like this PR: apart from the activity refactor, with vue we can finally unify UI and logic in a single file instead of putting the UI in global.js Also I like lines like export const translation = reactive({}); in such a way that translation is automatically updated.

locale/es-mx/translation.js Show resolved Hide resolved
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

npm run prettier

scripts/Activity.vue Outdated Show resolved Hide resolved
panleone
panleone previously approved these changes Sep 20, 2023
Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

re-tACK 00e1e70

JSKitty
JSKitty previously approved these changes Sep 20, 2023
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

tACK 76a8266

Smooth, almost identical functionality to Production, a really good cleanup for global.js, and a step towards MPW modularity

Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

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

tACK 190b044

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 190b044

Copy link
Member

@panleone panleone left a comment

Choose a reason for hiding this comment

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

tACK 190b044

@JSKitty JSKitty merged commit cd341de into PIVX-Labs:master Sep 21, 2023
1 check passed
JSKitty added a commit that referenced this pull request Sep 21, 2023
JSKitty added a commit that referenced this pull request Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Review This PR and/or issue is awaiting reviews before continuing. Enhancement New feature or request Review Reward: 10 PIV Reviewers of this Pull Request will receive a 10 PIV reward
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants