-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: Fix QA issues #327
fix: Fix QA issues #327
Conversation
…ponent and fix reverse previous error
src/components/tx/Transactions.js
Outdated
// We must use memo here because we were creating a new pagination | ||
// object in every new render, so the useEffect was being called forever |
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.
suggestion(non-blocking): Maybe we should change how PaginationURL
works.
The PaginationURL
use obtainQueryParams
to get the params specified in the contructor from the window.location.search
, so maybe we should have a way to get ts
, hash
and page
whenever location changes that is more in line with how function components work.
This obviously is outside the scope of this PR, so maybe we should open an issue for this discussion.
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 agree that it should be different from what it is nowadays. Maybe we could have an event thrown when parameters change. I will create an issue to discuss that, the idea of this PR is just to refactor the component to fix the bugs found.
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.
src/components/tx/Transactions.js
Outdated
function Transactions({ shouldUpdateList, updateData, title }) { | ||
// We must use memo here because we were creating a new pagination | ||
// object in every new render, so the useEffect was being called forever | ||
const pagination = useMemo( |
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.
question(if-minor): why not useState
?
I like the use of useMemo
, but this seems like a useState
without a setter, since this does not seem like a memoization and more like a singleton helper to calculate pagination params.
Something like: const [pagination, _] = useState(...)
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.
Make sense, done 05f8424
return () => { | ||
WebSocketHandler.removeListener('network', handleWebsocket); | ||
}; |
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.
[question] Is this unmounting really working? I'm asking this because React has probably a bug when dealing with unmounting when more then 1 useEffect
is declared in the component. If it is working, all good, otherwise I would solve this issue by declaring this useEffect
first.
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 tested and it's working fine when the screen is unmounted. What's this bug that React has with more than one useEffect
?
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.
Maybe this is a thing of React Native. When we use two useEffect
with unmounting
callbacks only the the first unmounting
callback from the first useEffect
is called, never the second. This case is little different because the first useEffect
didn't register an unmounting
, maybe you can make this test latter. However, for this PR it is enough. I'm approving.
Context
After @tuliomir ran the QA, 3 issues were found. They are described here.
componentDidUpdate
was not triggered when the query parameters changed. I decided to refactor the component to a functional component because this would be done in the future anyway, so I fixed the issue and did the refactor.Network
component right now.this.props
hadmatch: { peerId: undefined }
, so I had to fix the destructuring, so it could correctly get thepeerId
information.Note: While refactoring the
Transactions
component I found a bug that is affecting the mainnet-production explorer. When the user clicks the 'Previous' button in the list, the API returns the data in the reverse order, so we must reverse the data when this happens. In the middle of the refactor, I also fixed it.Note 2: I also found a new bug in the
Network
screen. The 'Reload data' button was not working because it stopped binding thethis
after the upgrades.Acceptance Criteria
Transactions
component to a functional component fixing the bug when the update was not being triggered when the query params changed.Transactions
component.peerId
query param.Security Checklist