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.

Sign up for GitHub

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

Jump to bottom

feat: revamp-tools-ui #345

Merged
merged 10 commits into from
Dec 9, 2024
Merged

feat: revamp-tools-ui #345

merged 10 commits into from
Dec 9, 2024

Conversation

lacerdamurilo
Copy link
Contributor

Acceptance Criteria

  • This PR includes the revamp of the following screens:
    • Decode TX
    • Push TX
    • DAG
    • Features
  • The GDPRConsent cookie alert has been fixed.

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.
DecodeTX.webm
DecodeTXMobile.webm
PushTX.webm
PushTXMobile.webm
DAG.webm
DAGMobile.webm
Feature.webm
FeatureMobile.webm

@tuliomir tuliomir self-assigned this Nov 25, 2024
@tuliomir tuliomir self-requested a review November 25, 2024 18:38
r4mmer
r4mmer reviewed Dec 3, 2024
View reviewed changes
Copy link
Member

@r4mmer r4mmer left a comment

Choose a reason for hiding this comment

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

There is also a small build error on file ./src/components/DropDetails.js.

src/components/DagComponent.js Outdated
Comment on lines 541 to 557
renderUi() {
return (
<div ref="dagWrapper" className="dagWrapper">
<svg id="graph" />
<div className="tooltip"></div>
</div>
);
}

renderNewUi() {
return (
<div ref="dagWrapper" className="dagWrapper">
<svg id="graph" />
<div className="tooltip"></div>
</div>
);
}
Copy link
Member

Choose a reason for hiding this comment

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

question: What changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point during development, we separated the returns, but we realized it wasn’t necessary. We’ve adjusted it now. Waiting for your review.

src/components/tx/TxData.js Outdated
Comment on lines 538 to 564
{renderListWithLinks(conflictNotTwin, true)}
{this.props.newUiEnabled
? renderNewUiListWithLinks(conflictNotTwin)
: renderListWithLinks(conflictNotTwin, true)}
Copy link
Member

Choose a reason for hiding this comment

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

Since this is used many times on this file, maybe the better way to do this would be to move the conditional to the renderListWithLinks so only the method changes and the rest of the file remains the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve applied your suggestion and refactored the code accordingly. Now, instead of calling renderNewUiListWithLinks multiple times, we only call renderListWithLinks and handle the validation of what it returns within the method itself. This should make the code cleaner and more maintainable. Let me know if you need any further adjustments!

r4mmer
r4mmer requested changes Dec 4, 2024
View reviewed changes
Copy link
Member

@r4mmer r4mmer left a comment

Choose a reason for hiding this comment

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

It seems the PR validation build step is failing with the error

./src/components/DropDetails.js
  Line 24:6:  React Hook useEffect has a missing dependency: 'startOpen'. Either include it or remove the dependency array  react-hooks/exhaustive-deps```

r4mmer
r4mmer reviewed Dec 4, 2024
View reviewed changes
src/components/DagComponent.js Outdated
@@ -366,6 +370,51 @@ class DagComponent extends React.Component {
});
}

newDrawGraph() {
Copy link
Member

Choose a reason for hiding this comment

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

I missed this during the first review, but this method is basically the same as drawGraph, why not use a single function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for catching this! You’re absolutely right, the logic in both methods was very similar and could be combined into a single function. I’ve refactored the code to use a single drawGraph function, separating the behaviors with conditional logic. This makes the code cleaner and easier to maintain. Let me know if there’s anything else you’d like me to adjust!

@r4mmer r4mmer mentioned this pull request Dec 4, 2024
Merged
1 task
tuliomir
tuliomir requested changes Dec 4, 2024
View reviewed changes
tuliomir
tuliomir previously approved these changes Dec 6, 2024
View reviewed changes
r4mmer
r4mmer reviewed Dec 9, 2024
View reviewed changes
src/components/feature_activation/Features.js Outdated
Comment on lines 238 to 240
hasBefore = () => this.state.page > 1;

hasAfter = () => this.state.page < this.state.pages.length;
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 already defined earlier in the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing that out! I've removed the redundant lines to avoid duplication.

@tuliomir tuliomir merged commit 76457e2 into HathorNetwork:master Dec 9, 2024
1 check passed
@NaanAndrade
Copy link

Melhor desenvolvedor, tive a oportunidade de trabalhar com ele e só tenho boas lembranças. Educado, inteligente e sabe liderar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuliomir tuliomir tuliomir approved these changes

@r4mmer r4mmer r4mmer approved these changes

Assignees

@tuliomir tuliomir

Labels
None yet
Projects
Hathor Network
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lacerdamurilo @NaanAndrade @tuliomir @r4mmer