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

Remove noUiSlider and use pi-ui one #3046

Merged
merged 6 commits into from
Mar 5, 2021

Conversation

guilhermemntt
Copy link
Contributor

This PR is a PoC of decred/pi-ui#308. It will solve #2874 when approved. For while, pi-ui dependence in package.json has been changed to my forked repository, but package.json changes will be discarded after the merge in the official pi-ui repo.

@guilhermemntt guilhermemntt marked this pull request as ready for review December 8, 2020 16:12
@tiagoalvesdulce
Copy link
Member

tiagoalvesdulce commented Dec 9, 2020

You may know it already but you can yarn link local packages to develop locally, run yarn start on pi-ui and you can see the changes made there on decrediton with hot reloading. One thing is that the react version on both repos have to be exactly the same. So, I usually do the following: go to /decrediton/node_modules/react, link it, go to /pi-ui and yarn link react, now link pi-ui and go to /decrediton and yarn link pi-ui.

@guilhermemntt
Copy link
Contributor Author

Thanks for your suggestion @tiagoalvesdulce! Luckily, just small changes have been required to fit pi-ui slider into Decrediton. By now, this PR is ready for review. Thank you all. I'll wait for the reviews.

@@ -286,8 +286,7 @@
"mv": "^2.1.1",
"node-abi": "^2.15.0",
"node-addon-loader": "decred/node-addon-loader#master",
"nouislider": "^12.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Copy link
Member

@vctt94 vctt94 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, looking good, but I believe I am hitting an infinite loading which makes my computer really slow if I play a bit with the slider

@guilhermemntt
Copy link
Contributor Author

Well, thanks for the feedback @vctt94, I'll take a look.

@guilhermemntt
Copy link
Contributor Author

It seems it's possible to achieve better performance using useCallback hook with lodash debounce. This way, we fetch from dcrwallet only after a 100 ms interval when sliding, avoiding infinite loadings, but this delay can take longer if needed.

@guilhermemntt
Copy link
Contributor Author

Rebased.

@jholdstock
Copy link
Member

I am getting errors when trying to run this with yarn dev. I can open the transaction history page, but when I try to open the "Sort" dropdown I get an electron console error:

Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function
(for composite components) but got: undefined. You likely forgot to export your component from the file it's
defined in, or you might have mixed up default and named imports.

Check the render method of `EyeFilterMenuWithSliderMenu`.
    in EyeFilterMenuWithSliderMenu (created by EyeFilterMenuWithSlider)
    in div (created by EyeFilterMenu)
    in div (created by EyeFilterMenu)
    in EyeFilterMenu (created by EyeFilterMenuWithSlider)
    in EyeFilterMenuWithSlider (created by Page)
    in div (created by Tooltip)
    in Tooltip (created by Page)
    ...

@guilhermemntt
Copy link
Contributor Author

Thanks for testing this one @jholdstock. Could not reproduce this warning here. Are you sure you reinstalled the dependencies using yarn before running this branch?

Copy link
Member

@jholdstock jholdstock left a comment

Choose a reason for hiding this comment

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

That was it, it works after running yarn. Looks good and works well.

The performance is absolutely appalling when loading transactions - the app becomes unresponsive, CPU spins, and animations chug, but that is a known issue which is noted elsewhere, and not related to this PR.

@guilhermemntt
Copy link
Contributor Author

Rebased.

package.json Outdated
@@ -286,8 +286,7 @@
"mv": "^2.1.1",
"node-abi": "^2.15.0",
"node-addon-loader": "decred/node-addon-loader#master",
"nouislider": "^12.0.0",
"pi-ui": "https://github.com/decred/pi-ui",
"pi-ui": "https://github.com/guilhermemntt/pi-ui#50b040d1ea0a5e83f6d2fd9d9f3a970282f23320",
Copy link
Member

Choose a reason for hiding this comment

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

This is a NACK for me. Needs to be in a decred owned repo. Or someone with longer standing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, @alexlyp, sorry. Guys at pi-ui will merge my PR there as soon this one here is approved, but I can talk to them. So I can use the master pi-ui repo and we can finally merge it if it's ok.

Copy link
Member

Choose a reason for hiding this comment

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

OK now that it's merged in pi-ui, please go ahead and remove this change and then should be good to go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Thanks @alexlyp

package.json Outdated
@@ -286,7 +286,8 @@
"mv": "^2.1.1",
"node-abi": "^2.15.0",
"node-addon-loader": "decred/node-addon-loader#master",
"pi-ui": "https://github.com/guilhermemntt/pi-ui#50b040d1ea0a5e83f6d2fd9d9f3a970282f23320",
"nouislider": "^12.0.0",
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 go away

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Thanks!

@alexlyp alexlyp merged commit 0b6302d into decred:master Mar 5, 2021
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.

6 participants