-
Notifications
You must be signed in to change notification settings - Fork 10
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
CHAL-9 #done Migrate the challenger welcome page #6
CHAL-9 #done Migrate the challenger welcome page #6
Conversation
Hey @dvorakjt I made the changes I wanted to make. Feel free to review it when you get a chance. |
Thanks @MazharulIslam-Naim! I will review it tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Naim,
Looks good so far, thanks for your hard work on this. There are a number of changes I need to request. Thanks in advance for implementing them!
- Please ensure your fork of the repo is up-to-date with the development branch of the 8by8 repo so that your changes can be merged once they are ready.
- The storybook story for this page is broken. You need to add some parameters to the meta object to instruct Storybook to use the Next.js app router. For more information, see this. At minimum, you will need to add the following:
const meta: Meta = { component: ChallengerWelcome, parameters: { nextjs: { appDirectory: true, }, }, };
- Once you do this, you will instead see a NullContextError. This is because the component has not been wrapped in a UserContext provider in the Storybook file. The UserContext is available throughout the app because it is included in the layout.tsx file at the root of the app directory. But for stories and tests, you should add it manually if your component depends on it. In this case, that's probably not actually necessary because we are going to remove all references to the UserContext from your component, but I wanted you to be aware of this.
- After reviewing the Figma, it's clear that this page is not rendered while the user is signed in. Therefore, we can remove all references to the UserContext from within the page. Once we have higher order AuthGuard components, we'll just wrap the entire thing in one of those. This will mean that the component doesn't need to be wrapped in a UserContext provider, and that your tests can be updated to reflect that.
- There is no need to set the user type in session storage, this can be removed entirely. We will need to perform some such operation when we get to player stories, but even that operation will not occur directly in a presentation-layer component such as this.
- Please use createNamedContext together with useContextSafely for the RewardsContext for consistency. This will mean that in testing, you will need to wrap the component in a RewardsContext provider.
- The call to useState to set rewardsAvailable is unnecessary. This can simply be declared like this:
const rewardsAvailable = rewards.some(r => r.rewardAvailable);
- In order to make the RewardsContext available to the application, please add its provider to the layout.tsx component in the app directory, after the UserContextProvider.
- There is probably no need to test the RewardsContext for now, as it doesn't really do anything except provide an empty array. You should be able to safely add it to the
coveragePathIgnorePatterns
array in thejest.config.mjs
file and remove your test file. Consider updating the comment to reflect that in the future, once it has some actual behavior, it should be removed from this array. - Check the styles against the Figma and try to use the spacing variables found in partials. Review the design guide portion of the Figma. Examine the page with a fine-toothed comb. In general, treat existing styles skeptically. Can they more closely match the Figma? In most cases, this is likely. A few things I noticed were:
a. margin between "see why others are doing it" and "How it works" should be the
same as the margin between "sign in" and "how it works" in the figma. The margin is too large
b. by default, most links have a font size of xs, this particular signin link seems to be xxs in the figma, note the difference in font size between the text before the link and after it - No need to import
useRouter
in the test or snapshot files. - Please use camelCase for image imports, as per the style guide.
- When I installed the dependencies for this PR, I got a security warning. Please run
npm audit
and if necessary,npm audit fix
to fix this vulnerability.
Keep up the great work, and thanks for your patience with my detailed code reviews. Thank you!
Fixed merge conflicts and redid rewards context. Let me know if there is anything I need to fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved.
Checklist
Overview
This PR creates the challengerwelcome page and the rewards context. There is also an npm package added, next-router-mock, for testing router changes caused by a button click.
Test Plan
There are snapshot tests for the challengerwelcome page and unit tests for the buttons on the page. The reward context is also tested.
Follow ups