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.

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

Add Voter Status to Network Configuration Tooltip (Popout) #2471

Merged
merged 11 commits into from
Mar 23, 2023

Conversation

jseagrave21
Copy link
Contributor

@jseagrave21 jseagrave21 commented Mar 15, 2023

What current issue(s) from Trello/Github does this address?
#2284

What problem does this PR solve?
Adds voter status to the Network Configuration Tooltip

How did you solve this problem?
Added calls to the voter and committee endpoints then displays them in the popout as recommended in the issue.

If a user has voted, the popout will look like this:
WithCandidateNodeSelected

If a user has not voted, the popout will look like this:
WithoutCandidateNodeSelected

How did you make sure your solution works?
yarn dev testing and updated unit tests

Are there any special changes in the code that we should be aware of?
All translations are updated.

Is there anything else we should know?
I did not implement a warning if the voted node has fallen out of the top 21. I can work on that if it is necessary. I was thinking that changing the color of the text to red would be enough of a warning but I would like some guidance if this is necessary before I put more work into figuring out how to do that.

The text displayed without a vote is hard coded in English. I could drop it so there is nothing displayed if that is preferable. Or please recommend a better solution.

  • Unit tests written? (updated)

To Do:

  • Fix WarningIcon color or leave as is
  • Update all translations
  • Write a test for all conditions of expected data for renderNode()
  • Fix NetworkConfigurationToolTip Container Size

@comountainclimber
Copy link
Member

This is awesome @jseagrave21 !

Some minor design feedback:

Screenshot 2023-03-15 at 11 56 12 AM

Screenshot 2023-03-15 at 11 55 13 AM

Lets ditch the row dedicated to the position of the node and instead display this pink icon as used frequently with this component => neon-wallet/app/components/AlertBox/index.jsx whenever the node falls "out of place"

Lets also display that same icon if no vote has been cast for the address

@jseagrave21
Copy link
Contributor Author

jseagrave21 commented Mar 16, 2023

@comountainclimber Thank you! Here is what I have so far.

Normal, with a node selected:
Normal Node

If the node has fallen outside of 21:
Fallen Outside Node

If there is no node selected:
No Node

I kept the node row after the public key because I thought the variation in opacity helped highlight the data.

I could not figure out how to update the color of the WarningIcon. Could you help me with that? Or we can keep it as is.

'To Do' list is updated to track outstanding tasks.

@jseagrave21
Copy link
Contributor Author

jseagrave21 commented Mar 17, 2023

I think this is good to go except for testing all the possibilities for renderNode.

I was not able to figure out how to pass state changes into the tests. I tried setting the state of VotedNode in initialState but that didn't work. I also tried to use wrapper.setState() after the instance of the tooltip has been mounted but that also didn't work. In all cases, with wrapper.text() I see 'VOTE CAST FOR:' with a blank after that. In the snapshot I can see that the WarningIcon is being displayed, but I cannot figure out how to pass a test array (e.g. ['test node', 1] into VotedNode and have it update in the test output.

@jseagrave21
Copy link
Contributor Author

Normal, with vote cast:
With Node Normal

If node drops out of the top 21:
If Node Drops Out

Without a vote cast:
No Node

const results = []
const { net, address } = this.props
const network = net === 'MainNet' ? 'mainnet' : 'testnet'
const data1 = await NeoRest.voter(address, network)
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: the variable name data1 is highly ambiguous perhaps "candateResponse" would be better here. Same goes for data2 I think something like "committeeResponse" would be slightly better

@comountainclimber
Copy link
Member

I think this is good to go except for testing all the possibilities for renderNode.

I was not able to figure out how to pass state changes into the tests. I tried setting the state of VotedNode in initialState but that didn't work. I also tried to use wrapper.setState() after the instance of the tooltip has been mounted but that also didn't work. In all cases, with wrapper.text() I see 'VOTE CAST FOR:' with a blank after that. In the snapshot I can see that the WarningIcon is being displayed, but I cannot figure out how to pass a test array (e.g. ['test node', 1] into VotedNode and have it update in the test output.

The following is how you would accomplish setting state in jest:

https://legacy.reactjs.org/docs/test-utils.html#act


  import { act } from 'react-dom/test-utils'
  
  test('Can call setState', () => {
    const { wrapper } = networkConfigTooltipSetup()
    console.log(wrapper.state()) // returns { isLoading: false };
    act(() => {
      wrapper.setState({ foo: 'bar' })
    })
     console.log(wrapper.state()) // returns { foo: 'bar };
  })

@comountainclimber
Copy link
Member

A few things:

  • Lets stick to the original order of the list that I suggested I think it looks less zebra like and confusing -

image

  • the icon and the text for the candidate should be vertically in-line for a cleaner look

  • The SVG color can easily be changed via CSS just like we do elsewhere in the application:

  svg {
    path {
      fill: #d355e7 !important;
    }
  }

This one is getting super close, after this stuff gets addressed I will approve this, thanks @jseagrave21 !

@jseagrave21
Copy link
Contributor Author

jseagrave21 commented Mar 21, 2023

@comountainclimber I still have not been able to figure out the testing but I resolved all the other issues. I will push the updated code with screenshots but I wanted to show you my test that is failing.

First, I was not able to get act to work so I decided to mock the api calls. Here is the test:

describe('Vote cast with candidate in top 21', () => {
  beforeEach(() => {
    jest
      .spyOn(NeoRest, 'voter')
      .mockImplementation(mockPromiseResolved({candidate: 'test node'}))
  })
    jest
      .spyOn(NeoRest, 'committee')
      .mockImplementation(mockPromiseResolved([{name: 'test node'}]))

  afterEach(() => {
    jest.restoreAllMocks()
  })

  describe('vote', () => {
    test('render', () => {
      const { wrapper } = networkConfigTooltipSetup()
      console.log(wrapper.text())
      expect(wrapper.text().includes('test node #1')).toBe(true)
    })
  })
})

I can see with logs that the mocks work as expected but the test fails before the state is updated. If I put in a timeOut or an await act that does not fix the issue. Can you help?

@jseagrave21
Copy link
Contributor Author

jseagrave21 commented Mar 21, 2023

Here is a screenshot with the updated order and alignment:
When Node Falls Out

One thing that I see is that your icon seems to be more aligned with the text but I am not sure how I could improve other than vertically aligning the icon with the bottom of the adjacent text which is what I did.

@jseagrave21
Copy link
Contributor Author

jseagrave21 commented Mar 22, 2023

@comountainclimber I refactored NetworkConfigurationTooltip using hooks and was able to write tests for renderNode(). I think everything should be good now.

Copy link
Member

@comountainclimber comountainclimber left a comment

Choose a reason for hiding this comment

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

🔥 🔥

@comountainclimber comountainclimber merged commit 5bd5c6a into CityOfZion:dev Mar 23, 2023
@jseagrave21 jseagrave21 deleted the voter_status branch March 23, 2023 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants