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

fix: improve readability of node info metrics value #7053

Merged
merged 5 commits into from
Jun 29, 2023
Merged

fix: improve readability of node info metrics value #7053

merged 5 commits into from
Jun 29, 2023

Conversation

evavirseda
Copy link
Collaborator

Summary

Closes #6899

Changelog

image

Testing

Platforms

Please select any platforms where your changes have been tested.

  • Desktop
    • MacOS
    • Linux
    • Windows
  • Mobile
    • iOS
    • Android

Instructions

Please describe the specific instructions, configurations, and/or test cases necessary to test and verify that your changes work as intended.

...

Checklist

Please tick the following boxes that are relevant to your changes.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or modified tests that prove my changes work as intended
  • I have verified that new and existing unit tests pass locally with my changes
  • I have verified that my latest changes pass CI workflows for testing and linting
  • I have made corresponding changes to the documentation

begonaalvarezd
begonaalvarezd requested changes Jun 28, 2023
View reviewed changes
Copy link
Member

@begonaalvarezd begonaalvarezd left a comment

Choose a reason for hiding this comment

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

Great work 💪🏼
added a minor comment to help improving the code 🙏🏼

packages/desktop/components/popups/NodeInfoPopup.svelte Outdated Show resolved Hide resolved
@begonaalvarezd begonaalvarezd changed the title Fix: Improve readability of node info metrics value fix: improve readability of node info metrics value Jun 28, 2023
begonaalvarezd
begonaalvarezd requested changes Jun 28, 2023
View reviewed changes
Copy link
Member

@begonaalvarezd begonaalvarezd left a comment

Choose a reason for hiding this comment

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

almost there 🌷

packages/desktop/components/popups/NodeInfoPopup.svelte Outdated Show resolved Hide resolved
cpl121
cpl121 reviewed Jun 28, 2023
View reviewed changes
Copy link
Member

@cpl121 cpl121 left a comment

Choose a reason for hiding this comment

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

When the rate information is copied, it appears in the form of a percentage (%), whereas when the entire information is copied, it appears in the previous format. I'm not sure if updating the nodeInfo store with the new format to maintain the same formatting would be correct

It's just a suggestion, as I'm not sure if what I'm saying is correct

@begonaalvarezd
Copy link
Member

When the rate information is copied, it appears in the form of a percentage (%), whereas when the entire information is copied, it appears in the previous format. I'm not sure if updating the nodeInfo store with the new format to maintain the same formatting would be correct

It's just a suggestion, as I'm not sure if what I'm saying is correct

I think its ok how it is now, we are beautifying it for presentational purposes but the copy information function uses a JSON that can be more technical and present the information directly fetched from the node

cpl121
cpl121 approved these changes Jun 29, 2023
View reviewed changes
Copy link
Member

@cpl121 cpl121 left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

begonaalvarezd
begonaalvarezd approved these changes Jun 29, 2023
View reviewed changes
Copy link
Member

@begonaalvarezd begonaalvarezd left a comment

Choose a reason for hiding this comment

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

🚀

@begonaalvarezd begonaalvarezd merged commit cc7963f into develop Jun 29, 2023
@begonaalvarezd begonaalvarezd deleted the feat/improve-readability-of-info-metrics branch June 29, 2023 08:27
msarcev pushed a commit that referenced this pull request Jul 4, 2023
@evavirseda @msarcev
fix: improve readability of node info metrics value (#7053)
f4ee6dd 
* fix: format node info metrics

* feta: improve code

* fix: use key instead of localeKey and round to 1 decimal

* fix: cleanup code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpl121 cpl121 cpl121 approved these changes

@begonaalvarezd begonaalvarezd begonaalvarezd approved these changes

@marc2332 marc2332 Awaiting requested review from marc2332

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Task]: Improve readability of node info metrics value
3 participants
@evavirseda @begonaalvarezd @cpl121