Feat: Add Mobile styles to FooterNav #181

haworku · 2020-05-18T22:34:07Z

`<FooterNav>` and `FooterExtendedNavList>`

Completed

Add mobile styles and behavior
Refactor out new FooterExtendedNavList
Add tests

Testing

View Footer, FooterNav, FooterExtendedNavList components in mobile styles.

haworku · 2020-05-18T22:34:30Z

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.test.tsx

+
+  describe('isMobile', () => {
+    it.todo(
+      'renders mobile view when client window width is less than threshold'


I can't figure out how to test window resize with running into typescript read-only limitations on reassigning window objects during a test. Ideally, I was going to use jest beforeAll to adjust the window size to mobile before running all the tests in this describe block.

could you use the resizeTo method instead of reassigning the dimensions? https://developer.mozilla.org/en-US/docs/Web/API/Window/resizeTo

imo this would be a great test to have!

Tried out resizeTo but its not implemented. jest incorporates JSDOM to override window - and JSDOM doesn't support all the same methods. Ended up overriding tyepscript and then eslint warnings about overriding typescript 😆 I hadn't realized I could just do that and move on.

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.stories.tsx

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.tsx

src/components/Footer/FooterNav/FooterNav.tsx

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.tsx

eamahanna

I just had a couple of questions. Nice work :)

- refactor out FooterExtendedNavList - add more tests

suzubara

I left some comments but they are pretty minor. This is looking really good!

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.stories.tsx

suzubara · 2020-05-20T16:23:40Z

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.test.tsx

+
+  describe('isMobile', () => {
+    it.todo(
+      'renders mobile view when client window width is less than threshold'


could you use the resizeTo method instead of reassigning the dimensions? https://developer.mozilla.org/en-US/docs/Web/API/Window/resizeTo

imo this would be a great test to have!

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.tsx

suzubara · 2020-05-20T16:29:04Z

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.tsx

+
+    function handleResize(): void {
+      const isMobileWidth = window.innerWidth < 480
+      setIsMobile(isMobileWidth)


my only suggestion here would be to only call setIsMobile if the value has changed, to avoid extraneous state updates

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.tsx

suzubara · 2020-05-20T16:39:34Z

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.tsx

+  React.HTMLAttributes<HTMLElement>): React.ReactElement => {
+  const classes = classnames('grid-row grid-gap-4', className)
+
+  const [isMobile, setIsMobile] = React.useState(false)


This is super minor but I noticed there is a brief flash of the desktop nav at mobile sizes on render. I think that could be resolved by doing the initial window size check here to default the state to the correct value (instead of false) and then that would remove the need to trigger handleResize() in the effect hook

- add back useEffect for window resize - make isMobile be possibly undefined - adjust tests and remove unncessary story - prevent unncessary state renders

haworku · 2020-05-21T14:07:17Z

@suzubara I incorporated the changes you suggested. Tried to rename variables a bit to help clarify the difference between isMobile the prop and then the fallback state based on screen width.

suzubara

left a couple comments but only one is blocking

suzubara · 2020-05-21T15:54:29Z

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.test.tsx

+        // Mobile width is less than 480
+        //eslint-disable-next-line @typescript-eslint/ban-ts-ignore
+        //@ts-ignore
+        window.innerWidth = 479


non-blocking: just curious, did you try using window.resizeTo instead of assigning innerWidth? then I don't think we'd run into the TS problem

FYI #181 (comment) Also just double checked it this is the exact error in the console Error: Not implemented: window.resizeTo on running jest.

Ooh I see, got it! 👍

suzubara · 2020-05-21T15:59:14Z

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.tsx

+    if (!isClient) return
+
+    function handleResize(): void {
+      if (isMobileWidth !== isMobileFallback) {


I think isMobileWidth needs to be re-checked on resize? It doesn't look like this changes when you size up/down

Nice catch I wasn't seeing any issues with my testing and this made me look at again. Definitely need to called window.innerWidth each time in handleResize to get the right behaviors when lots of resizing of the window is happening.

suzubara · 2020-05-21T16:02:32Z

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.tsx

+  const useMobile = isMobile || (isMobile === undefined && isMobileFallback)
+
+  useEffect(() => {
+    if (!isClient) return


non-blocking: just for performance, couldn't we also do an early return if isMobile is defined? since then the fallback isn't used at all and we wouldn't need to attach the event listener.

- refactor out FooterExtendedNavList - add more tests

haworku requested review from gidjin, sarboc, suzubara and tinyels as code owners May 18, 2020 22:34

haworku commented May 18, 2020

View reviewed changes

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.stories.tsx Outdated Show resolved Hide resolved

haworku commented May 18, 2020

View reviewed changes

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.tsx Outdated Show resolved Hide resolved

haworku requested a review from eamahanna May 18, 2020 22:52

haworku commented May 18, 2020

View reviewed changes

src/components/Footer/FooterNav/FooterNav.tsx Show resolved Hide resolved

eamahanna reviewed May 19, 2020

View reviewed changes

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.tsx Show resolved Hide resolved

eamahanna reviewed May 19, 2020

View reviewed changes

src/components/Footer/FooterExtendedNavList/FooterExtendedNavList.tsx Outdated Show resolved Hide resolved

eamahanna reviewed May 19, 2020

View reviewed changes

haworku force-pushed the hw-footer-mobile branch from b8daa18 to 50af0e6 Compare May 19, 2020 21:58

haworku force-pushed the hw-footer branch from d7a3e6e to e4aabcc Compare May 19, 2020 22:05

haworku force-pushed the hw-footer-mobile branch from 50af0e6 to 3e0e4b2 Compare May 19, 2020 22:16

feat: add mobile styles to Footer

a22f434

- refactor out FooterExtendedNavList - add more tests

haworku force-pushed the hw-footer-mobile branch from 3e0e4b2 to a22f434 Compare May 20, 2020 13:55

haworku added 2 commits May 20, 2020 09:46

fix: add footerSecondary to NavList

c10f495

fix: use isMobile as prop

a299f35

suzubara reviewed May 20, 2020

View reviewed changes

fix: code review

593e90a

- add back useEffect for window resize - make isMobile be possibly undefined - adjust tests and remove unncessary story - prevent unncessary state renders

haworku added 3 commits May 21, 2020 09:21

fix: add mobile screen size tests

7a9df0a

fix: define isClient once

4389a6d

fix: cleanup and organize tests further, bc why not?

14e7c34

haworku requested a review from suzubara May 21, 2020 14:51

suzubara reviewed May 21, 2020

View reviewed changes

haworku added 2 commits May 21, 2020 13:48

fix: useEffect check

18800b0

fix: make sure window.innerWidth is always recalculated in handleResize

b25d3bd

haworku requested a review from suzubara May 21, 2020 19:08

suzubara approved these changes May 22, 2020

View reviewed changes

haworku merged commit 188ce99 into hw-footer May 22, 2020

haworku deleted the hw-footer-mobile branch May 22, 2020 16:17

haworku added a commit that referenced this pull request May 22, 2020

Feat: Add Mobile styles to FooterNav (#181)

ff87459

- refactor out FooterExtendedNavList - add more tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add Mobile styles to FooterNav #181

Feat: Add Mobile styles to FooterNav #181

haworku commented May 18, 2020

haworku May 18, 2020 •

edited

Loading

suzubara May 20, 2020

haworku May 21, 2020 •

edited

Loading

eamahanna left a comment

suzubara left a comment

suzubara May 20, 2020

suzubara May 20, 2020

suzubara May 20, 2020

haworku commented May 21, 2020 •

edited

Loading

suzubara left a comment

suzubara May 21, 2020

haworku May 21, 2020

suzubara May 22, 2020

suzubara May 21, 2020

haworku May 21, 2020

suzubara May 21, 2020

Feat: Add Mobile styles to FooterNav #181

Feat: Add Mobile styles to FooterNav #181

Conversation

haworku commented May 18, 2020

<FooterNav> and FooterExtendedNavList>

Completed

Testing

haworku May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haworku May 21, 2020 • edited Loading

Choose a reason for hiding this comment

eamahanna left a comment

Choose a reason for hiding this comment

suzubara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haworku commented May 21, 2020 • edited Loading

suzubara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`<FooterNav>` and `FooterExtendedNavList>`

haworku May 18, 2020 •

edited

Loading

haworku May 21, 2020 •

edited

Loading

haworku commented May 21, 2020 •

edited

Loading