This repository has been archived by the owner on Dec 20, 2024. It is now read-only.
Contra Frontend technical assessment #64
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TS-interview
commented
Sep 3, 2022
| '^@/components/(.*)$': '<rootDir>/src/components/$1', | ||
| '^@/hooks/(.*)$': '<rootDir>/src/hooks/$1', | ||
| '^@/pages/(.*)$': '<rootDir>/src/pages/$1', | ||
| }, |
There was a problem hiding this comment.
It would help to have this setup before interviewees start on the project.
Looks like this will be added by the Next team in the near future (link)
TS-interview
commented
Sep 3, 2022
| "eslint": "8.18.0", | ||
| "eslint-config-canonical": "35.0.1", | ||
| "eslint-config-next": "12.1.6", | ||
| "eslint-config-prettier": "8.5.0", | ||
| "jest": "28.1.1", | ||
| "jest-environment-jsdom": "^29.0.1", |
There was a problem hiding this comment.
Would appreciate having this dependency pre-installed as well 😄
TS-interview
commented
Sep 3, 2022
|
|
||
| const closeHandler = useCallback(() => { | ||
| setOpen(false); | ||
| }, []); |
There was a problem hiding this comment.
Want to preface that preemptive performance changes are an anti-pattern, and these changes usually should be made after doing an internal investigation/audit (via 3rd party software like lighthouse).
TS-interview
commented
Sep 3, 2022
| const container = screen.getByTestId('overflow-container'); | ||
| fireEvent.scroll(container, { target: { scrollY: 100 } }); | ||
| expect(callback).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
I'm not a fan of pushing skipped/broken tests, but for the sake of the assessment, I left it in.
Please forgive me 🙇
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Had a lot of fun working on this. I usually use pre-built modals, so I didn't realize how much I took that for granted. Especially the accessibility support. This was honestly my first time developing/testing with a screen reader, and it won't be the last.
Design decisions
Decided to use
styled-componentsto quickly create necessary styled components. If I were to scale this, I'd probably use a css framework like tailwind or chakra ui.I also left the css within the js as I didn't have that many custom styled components, and keep everything in one place. If I were to keep using styled components, I'd probably audit all current components and create common re-usable components.
Changes made
focus-trap-reactlibrary.ariaprops.Next steps
@/components/modal.tsxand@/pages/index.tsx. Maybe cypress tests for an example critical flow with a modal (popup alert or a form within the modal).Loom
Link here