-
Notifications
You must be signed in to change notification settings - Fork 985
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
Fixes #12192 -- Disable fetching more messages when status nodes are … #13450
Conversation
…odes are disabled
Hey @vampirekiddo, 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 |
Jenkins BuildsClick to see older builds (6)
|
@vampirekiddo thanks for the PR, great work! I have checked the UI and it seems they don't show as disabled when a the user is not using status nodes, although it does not fetch history anymore (which is the desired behavior) Basically they should show the same as when you are offline (Fetch more buttons are disabled), would you mind taking a look? The subscription I believe is using is https://github.com/status-im/status-react/blob/366f235e31fabd6c5869e67766a09acfcd1301ac/src/status_im/subs.cljs#L2494 , so that's one place where you might want to take a look. Thank you |
Thanks for the explanation Andrea! |
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.
Looks good, just a style issue, thank you!
first-gap? (= gap-ids #{:first-gap})] | ||
(when (or (not first-gap?) public? community?) | ||
[react/view {:style (style/gap-container)} | ||
[react/touchable-highlight | ||
{:on-press (when (and connected? (not in-progress?)) | ||
{:on-press (when (and connected? (and (not in-progress?) use-status-nodes?)) |
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 think you can simplify, as and
is a multi-variadic function, so you can do:
(and
connected?
(not in-progress?)
use-status-nodes?)
Which should be equivalent to:
(and
connected?
(and
(not in-progress?)
use-status-nodes?))
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.
Thanks Andrea
Removed the redundant and
function
74% of end-end tests have passed
Failed tests (22)Click to expand
Passed tests (62)Click to expand
|
(if (= gap-ids #{:first-gap}) | ||
(sync-chat-from-sync-from chat-id) | ||
(fill-gaps chat-id gap-ids)))) | ||
(let [use-status-nodes? (mailserver/fetch-use-mailservers? {:db db})] |
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.
when-let
could be used
95% of end-end tests have passed
Failed tests (1)Click to expand
Passed tests (21)Click to expand
|
@vampirekiddo thanx for your contribution. Ready for merge. |
…disabled
Fixes #12192
Summary
Added a boolean to check if status nodes are enabled or not and if they're disabled we disable the functionality of fetch more messages button
The issue didn't state to hide the fetch more messages button or to disable it
So I disabled it.
Platforms to test
Areas that maybe impacted
Functional
Steps to test
Go to settings -> sync settings and disable status nodes and then try to fetch more messages.
It won't work