-
Notifications
You must be signed in to change notification settings - Fork 984
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
[feature] [#13099] Automatic RPC usage refresh #13364
[feature] [#13099] Automatic RPC usage refresh #13364
Conversation
Hey @smohamedjavid, and thank you so much for making your first pull request in status-react! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2 |
|
||
(def rpc-usage-refresh-interval 2000) | ||
|
||
(reset! rpc-refresh-interval (utils/set-interval #(re-frame/dispatch [::get-stats]) rpc-usage-refresh-interval)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess we need this only when the screen is opened
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, probably best to use the component lifecycle methods (componentdidmount
& componentwillunmount
for cleaning up), there should be a few examples on how we do this in the codebase (if you search for component-did-mount
you should find a few examples)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flexsurfer @cammellos - Updated the code with interval cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smohamedjavid could you run make lint-fix
and make lint
please? The build is failing because of indentation.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cammellos - Apologies, I have fixed the lint issue and the build is successful now.
Jenkins BuildsClick to see older builds (12)
|
1001d5a
to
467a87a
Compare
|
||
(let [stats @(re-frame/subscribe [:rpc-usage/data]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you used letsubs , you need to move subsciptions to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a small hack to make sure the UI is updated when we dispatch the get-stats method. Apologies, I'll create a container and push it.
467a87a
to
ad3752a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
@@ -565,7 +565,7 @@ | |||
:component network-info/network-info} | |||
{:name :rpc-usage-info | |||
:options {:topBar {:title {:text (i18n/label :t/rpc-usage-info)}}} | |||
:component rpc-usage-info/usage-info} | |||
:component rpc-usage-info/usage-info-container} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its better to keep this name for container and rename usage-info to usage-info-render
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so here we should keep rpc-usage-info/usage-info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially started with that naming convention but I looked at other files where it follows as above so I followed the same (doesn't want to deviate from the code ambience). I can change the name if needed. Kindly advice. @flexsurfer
90% of end-end tests have passed
Not executed tests (1)Failed tests (8)Click to expand
Passed tests (76)Click to expand
|
100% of end-end tests have passed
Passed tests (2)Click to expand
|
Hi @smohamedjavid, thanks for your contribution! ISSUE 1: If a method has a long name its value is not displayed on AndroidAs far as I can see, this only applies to the method in the screenshot. If you copy it, it's pasted correctly, so it's absolutely not critical. |
@qoqobolo Apologies, I cannot run the project on Android emulators from my machine at the moment. However, I know the cause of this issue (due to flex styling of text) and I fixed it. Kindly test it. |
@smohamedjavid many thanks for this additional fix! Looks great now. |
Fixes #13099
Summary
In this PR, I have added an automatic refresh of RPC usage under RPC usage stats screen without the user's need to interact with the app.
This is achieved by calling the get-stats method every 2 seconds with the help of the set-interval method from utils.
I have removed the Refresh button from the UI as it no longer serves the purpose and kept the Reset and Copy buttons intact.
Areas that maybe impacted
Steps to test
status: ready