-
Notifications
You must be signed in to change notification settings - Fork 9
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-4 #done #3
CHAL-4 #done #3
Conversation
non-windows development
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 Joey,
Looks great. Good job naming things clearly, using semantically appropriate html elements and calling attention to follow-ups. Thank you for fixing the z-index issue with the PageContainer component.
I have created an issue in Jira as a reminder for us to get that speech bubble image updated. I'll get a designer to do that this week.
In terms of this PR, there are a few minor changes I'd like to request:
- Please replace any apostrophes in text within a JSX element with the appropriate escape character
'
. Please see the following ESLint rule for more information: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unescaped-entities.md - Please ensure that the code is formatted with Prettier. The indentation size should be 2 spaces if the Prettier has successfully formatted the code.
- For consistency, please use snake_case for SCSS class names. This casing pairs well with both SCSS module syntax and short utility class names like
mt_lg
Finally, I noticed this class name:
custom_shape_divider_top_1659139317
I'm not sure what the number at the end means. If it doesn't have any value to the reader, perhaps it could be shortened to "custom_shape_divider_top" ?
Excellent first contribution, thank you! Please make the above changes and we'll be able to merge it!
Hi Joseph, Thank you for the review and suggestions. I went ahead and made the appropriate changes. |
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.
Looks great, thanks for making the requested changes.
Checklist
Overview
This PR migrates the existing Why8by8 page to Next.js. It updates old image paths to new ones, replaces and tags to Next.js components and , and replaces background styling to use partials. Additionally, I added a border around the svg image, which was causing a thin teal line on some browsers, and also added styling to the page-container component to fix image overlapping the header. There were some issues with storybook which were worked around. The first in
.storybook/main.ts
was'..\\public'
didn't work as expected in a Linux environment. The second intsconfig.json
storybook couldn't locate alias paths, e.g.@/app/why8by8/page
Test Plan
I created a storybook story in
src/stories/pages
and a jest snapshot test insrc/__tests__/pages/why8by8
to ensure my component rendered and worked.Follow ups
The black background was replaced by
$color-black-8by8
partial, but the vote bubble image background wasn't changed, causing a difference in colors.