5430 other petitioners ui #5570
Conversation
sutt0n
commented
Jun 22, 2020
•
sutt0n
commented
…f-cms into 5430-other-petitioners-ui
…-other-petitioners-ui
| expect(result.showOtherPetitioners).toEqual(false); | ||
| }); | ||
|
|
||
| it('paginates if showingAdditionalPetitioners is false', () => { |
There was a problem hiding this comment.
How about if showingAdditionalPetitioners is false, but there aren't enough otherPetitioners to paginate?
There was a problem hiding this comment.
There's no logic to check the length to paginate or not. It's a "show 4 or show all" kinda thing
| > | ||
| <address aria-labelledby="secondary-label"> | ||
| {otherPetitioner.name && | ||
| OtherPetitionerDisplay(otherPetitioner, constants)} |
There was a problem hiding this comment.
I find this passing through of constants in props a bit strange. Could you just import them from state directly in the component where you need them?
There was a problem hiding this comment.
This was copied and pasted after I consolidated the AddressDisplay components that was defined in two differently spots, but yeah, I'll do that. 😄
| toggleShowAdditionalPetitionersSequence(); | ||
| }} | ||
| > | ||
| <FontAwesomeIcon icon={['far', 'address-card']} size="sm" /> |
There was a problem hiding this comment.
You can include this icon on the Button rather than directly importing the FontAwesomeIcon here.
| const showingAdditionalPetitioners = | ||
| get(state.showingAdditionalPetitioners) || false; | ||
|
|
||
| store.set(state.showingAdditionalPetitioners, !showingAdditionalPetitioners); |
There was a problem hiding this comment.
I think there's a store.toggle method for just this purpose -- assuming the tests pass, means fewer lines of code, better coverage by tiny percentages. :D
| <p> | ||
| <span className="address-line">{contact.address1}</span> |
There was a problem hiding this comment.
nit: For both this and the OtherPetitionerDisplay, this could be a good opportunity to create a common component for AddressDisplay. Perhaps then this component could be named ContactDisplay? Devex, if you like.
