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

Actions page ported over with typescript. #234

Open
wants to merge 2 commits into
base: next-js-migration
Choose a base branch
from

Conversation

MazharulIslam-Naim
Copy link
Member

Overview
This PR migrates the actions page and confetti component to next.js and adds tests for this page and component.

Test Plan
I wrote snapshot tests and unit tests for the buttons.

Follow ups

</div>


{showVoterFormModal && (
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm having trouble testing this component. The two tests I wrote for this specific part error because in the tests I try to open the modal and its calling the PopModal component.

Copy link
Contributor

@dvorakjt dvorakjt Feb 5, 2024

Choose a reason for hiding this comment

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

Hello Naim,

Thank you for bringing this up. The issue is that, although the dialog element is supported in modern browsers, and it has been used here for accessibility reasons, jsdom does not currently support it. Please see this thread for more information: jsdom/jsdom#3294

A workaround is to add the showModal property to HTMLDialogElement.prototype prior to executing the tests.

A simple example of this can be found in the snapshot file for the error-modal, which can be found in src/__tests__/components/error-modal.

I also like this implementation: jsdom/jsdom#3294 (comment)

Please let me know if there are any other difficulties I can help address. Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks this worked.

@ty730 ty730 requested a review from dvorakjt January 30, 2024 00:53
Copy link
Contributor

@dvorakjt dvorakjt left a comment

Choose a reason for hiding this comment

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

Hi Naim,

Thank you for your hard work on this PR and my apologies for the delay in reviewing it.

There is a lot of great stuff here! Sorry in advance for the length of my reply, but since you put a lot of work into this PR, I feel I owe it you to review it with equal care! First the high level stuff:

It would be awesome if we could our file naming conventions as consistent as possible. For now, let's go with all lowercase characters separated by hyphens. It would also be great to avoid abbreviations. This should be easy for everyone to read, and naming files consistently like this will help make it easier for new volunteers to find the files they need and understand what those files do.

Looking at the snapshot files created for actions, I wonder if they might be better implemented as a single spec file. My personal feeling is that snapshot tests are best used to check the appearance of static or at least relatively static pages, whereas spec files should be used to check behavior. Given that the names of the snapshot files, it's clear that these are actually being used to test behavior, in this case that various components are rendered or not depending on a set of conditions. Grouping these into a single spec file and then writing separate tests within one describe block to check for each component's presence will be much clearer to read, and you'll be able to avoid the lengthy and convoluted file names necessitated by separating these conceptually similar tests into separate files.

Moving onto more in-depth notes, in Rewards.ts, I think it would be better to export Reward itself as a type alias and then declare an Array of that type where needed. This is a more flexible approach, as it allows you to use the type Reward on its own, divorced from the array containing it (which will therefore also lead to DRYer code as you won't need to re-declare Reward elsewhere). Further, I believe its actually easier to read Array vs Rewards (with Array, you know there is no extra data associated with the type. With 'Rewards' you need to go find the type declaration to be sure).

Following this logic, in rewards-context.ts, I think its actually easier to read Array | null, rather than declaring a specific RewardsInfo type. But I also wonder if in this case, you could simply provide a default value of an empty array rather than null. Then you could simply provide declare the type of rewardsInfo as Array. In this case, it might also be best to rename the rewardsInfo variable in this context to rewards, for consistency.

Continuing this line of thought, simplifying the RewardsInfo type may simplify the checks you perform inside the useEffect hook in actions.tsx. For instance, you could probably inline the first check:

//here, rewards is of type Array<Reward> instead of Array<Reward> | null
const { rewards } = useContext(RewardsContext); 
const [rewardsAvailable, setRewardsAvailable] = useState(rewards.some(r => r.rewardAvailable));

I also wonder if some of the jsx in this file could be broken down into smaller components to improve readability?

Finally, a few other little things I noticed:

  • In line 442 in actions, the ariaLabel says "something went wrong." This does not seem to correspond with the intent of the modal?
  • In line 20 of confetti-animation.spec.ts, there is no ".to()" following the expect clause. Was this intentional?

Thank you very much in advance for making these changes. I may have a few more suggestions once these changes have been made. By making these changes, you contributing to an extremely clean, easy-to-work-with, thoroughly tested codebase. Thank you!

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