Skip to content
This repository has been archived by the owner on Oct 30, 2023. It is now read-only.

Feat / Refactor perils and add tab navigation #659

Merged
merged 12 commits into from
May 6, 2022

Conversation

gustaveen
Copy link
Member

@gustaveen gustaveen commented May 3, 2022

What?

Add tab navigation for Perils instead of current dropdown navigation.
I moved over the Tabs component from web-onboarding.

  • Add framer-motion dependency to make Tabs work. Our setup doesn't support versions after 4.1.17 (issue described here: [BUG] 5.0 Failed to compile with create-react-app framer/motion#1307) and that's the same version we use in onboarding. That did cause another issue as well where tslib complained WARNING in ./node_modules/popmotion/dist/es/utils/mix-complex.js 22:15-28 "export '__spreadArray' was not found in 'tslib' so we had to add that as a dependency as well. More on that issue here:
    [BUG] framer-motion breaks in tslib version 2.2.0 framer/motion#1123
  • Refactor Perils component to simply accept a collection of perils
  • Update the data structure to include an id and label for the tabs. I called it PerilsCollection. I therefore renamed some of the components to make more sense.
  • Update usePerils to fetch multiple peril types
  • Update PerilsBlock to pass perilsCollections

Why?

Consistent UI with Perils in onboarding

Screenshots / recordings

Storybook

Screen.Recording.2022-05-03.at.15.44.06.mov

Storyblok

Screen.Recording.2022-05-04.at.10.27.41.mov

@gustaveen gustaveen force-pushed the GRW-1047/refactor-perils-add-tabs branch 3 times, most recently from ce075d2 to 4950000 Compare May 4, 2022 08:26
@gustaveen gustaveen marked this pull request as ready for review May 4, 2022 08:30
@gustaveen gustaveen requested a review from a team as a code owner May 4, 2022 08:30
simonauner
simonauner previously approved these changes May 4, 2022
Copy link
Contributor

@simonauner simonauner left a comment

Choose a reason for hiding this comment

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

Nice!

I realized after adding comments that you probably just copy-pasted from web-onboarding without any regards to improving the code, in which case you can just disregard my suggestions 😄

src/components/Perils/PerilList/PerilList.tsx Outdated Show resolved Hide resolved
src/components/Perils/PerilList/PerilList.tsx Outdated Show resolved Hide resolved
src/components/Perils/types.ts Outdated Show resolved Hide resolved
@gustaveen
Copy link
Member Author

Nice!

I realized after adding comments that you probably just copy-pasted from web-onboarding without any regards to improving the code, in which case you can just disregard my suggestions 😄

No that's great! It was just me who missed it so thanks for pointing it out 🙏

@gustaveen gustaveen requested a review from simonauner May 4, 2022 15:12
simonauner
simonauner previously approved these changes May 5, 2022
Copy link
Contributor

@simonauner simonauner left a comment

Choose a reason for hiding this comment

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

Nice! A lint warning that could be nice to get rid of, otherwise LGTM!

src/components/Perils/data/usePerils.ts Outdated Show resolved Hide resolved
@gustaveen gustaveen requested a review from simonauner May 5, 2022 09:08
simonauner
simonauner previously approved these changes May 6, 2022
robinandeer
robinandeer previously approved these changes May 6, 2022
Copy link
Contributor

@robinandeer robinandeer left a comment

Choose a reason for hiding this comment

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

Nice work! This is almost nostalgic - I remember how hard/long I worked on this tab solution to make it accessible and get the damn line to animate between the tabs 😅

src/components/Perils/PerilList/PerilList.tsx Outdated Show resolved Hide resolved
src/components/Perils/PerilList/PerilList.tsx Show resolved Hide resolved
import { LAPTOP_BP_UP } from '../blockHelpers'
import { UnderlineComponent, UNDERLINE_HEIGHT } from './Underline'

const TabContainer = styled.button<{ selected?: boolean }>`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we override the focus state globally? Don't remember, otherwise would be nice to add one here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean adding a focus state for the tabs? @robinandeer

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah for these buttons -- but maybe it's covered in some other way?

src/components/Tabs/Underline.tsx Outdated Show resolved Hide resolved
@gustaveen gustaveen dismissed stale reviews from robinandeer and simonauner via 07c5228 May 6, 2022 08:45
@gustaveen gustaveen force-pushed the GRW-1047/refactor-perils-add-tabs branch from 067f832 to 07c5228 Compare May 6, 2022 08:45
@gustaveen
Copy link
Member Author

Thanks haha but you did all the heavy lifting for this one :D A tricky thing indeed with the line animation! 🙏 @robinandeer

@gustaveen gustaveen requested a review from robinandeer May 6, 2022 09:20
Copy link
Contributor

@robinandeer robinandeer left a comment

Choose a reason for hiding this comment

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

Cool stuff!

@gustaveen gustaveen merged commit a0eac99 into master May 6, 2022
@gustaveen gustaveen deleted the GRW-1047/refactor-perils-add-tabs branch May 6, 2022 12:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants