feat(OnboardingChecklist): new component #1270
Conversation
| export interface IOnboardingChecklistProps { | ||
| className?: string; | ||
| title: string; | ||
| titleLabel?: string; |
There was a problem hiding this comment.
Better to rename it
GreetingText: This name clearly indicates that the text is a greeting or welcome message.
| } | ||
| } | ||
|
|
||
| @media not (width >= 920px) { |
There was a problem hiding this comment.
Can be simplified to: (max-width: 919px)
| display: flex; | ||
| flex-direction: row; | ||
| gap: var(--spacing-5); | ||
| padding: 20px; |
There was a problem hiding this comment.
space
| padding: 20px; | |
| padding: var(--spacing-5); |
|
|
||
| &__column { | ||
| position: relative; | ||
| padding: 40px; |
There was a problem hiding this comment.
| padding: 40px; | |
| padding: var(--spacing-10); |
| const currentHeight = container.offsetHeight; | ||
| setCurrentContainerHeight(currentHeight); | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
we need to clear timeouts afterward
| completeItem, | ||
| className, | ||
| }) => { | ||
| const [complete, setComplete] = React.useState(isCompleted); |
There was a problem hiding this comment.
The name of this state can be confused with an external prop. Can you please rename is for better readability?
| }; | ||
|
|
||
| React.useEffect(() => { | ||
| const delay = completeItem?.delay || 1500; |
There was a problem hiding this comment.
why the completeItem marked here as optional, in the component's interface it is marked as required?
| import styles from './OnboardingChecklist.module.scss'; | ||
|
|
||
| const baseClass = 'onboarding-checklist'; | ||
| const COMPLETE_CONTAINER_EIGHT = 96; |
There was a problem hiding this comment.
| const COMPLETE_CONTAINER_EIGHT = 96; | |
| const COMPLETE_CONTAINER_HEIGHT = 96; |
| !complete || isOpen | ||
| ? currentContainerHeight | ||
| : COMPLETE_CONTAINER_EIGHT, |
There was a problem hiding this comment.
Will the completeStep always have the same height(COMPLETE_CONTAINER_EIGHT)? Because data from completeItem is dynamic
| items.map((item, index) => { | ||
| return ( | ||
| item.id === activeId && ( | ||
| <div | ||
| key={index} | ||
| className={styles[`${baseClass}__placeholder`]} | ||
| > | ||
| {item.placeholder} | ||
| </div> | ||
| ) | ||
| ); | ||
| })} |
There was a problem hiding this comment.
iterating through the whole array to display just one selected element is not the best idea. You can get this element by its Id and show it here
| ) | ||
| ); | ||
| })} | ||
| {isCompleted && completeItem && ( |
There was a problem hiding this comment.
you do not need to check for completeItem as it is not marked as optional
…sign-system into feature/onboarding-checklist
|
|
||
| export interface IOnboardingChecklistProps { | ||
| className?: string; | ||
| title: string; |
There was a problem hiding this comment.
Title: This name suggests that the text is the heading or title of the guide.
Resolves: #{issue-number}
Description
Storybook
https://feature-onboarding-checklist--613a8e945a5665003a05113b.chromatic.com/?path=/story/components-onboardingchecklist--default
Checklist
Obligatory: