feat: SB-2466 - feedback component#6
Conversation
poengen
left a comment
poengen
left a comment
There was a problem hiding this comment.
Overall a good structure with the compound component pattern. I think there's some confusion with the state handling and possibly room for some cleanup.
Proposed change: Remove the variant-prop on the Feedback component.
| size, | ||
| className, | ||
| children, | ||
| onClick, |
There was a problem hiding this comment.
This onClick prop should be used to set which child component is active. Either idle, collecting or submitted.
There was a problem hiding this comment.
Was technically not required, but it does make it easier to read, so this has now been changed.
There was a problem hiding this comment.
The code runs without it, but its a good practice to use it
| label: 'Feedback', | ||
| description: 'Form for submitting feedback', | ||
| el: ( | ||
| <Feedback> |
There was a problem hiding this comment.
Here you don't make use of the variant-prop. But this example seems to work also.
There was a problem hiding this comment.
Variant as a prop has been removed. Variant in other places has been renamed for clarity. For example as selectedFeedbackType to make it more obvious what this variant actually is.
| <Feedback.Idle> | ||
| <Heading size='small'>How was this page to use?</Heading> | ||
| <Feedback.Idle.ButtonGroup aria-label='Rate this page'> | ||
| <Feedback.Idle.Button size='large' variant='happy'>Good</Feedback.Idle.Button> |
There was a problem hiding this comment.
You should use an arrow function here in the onClick prop to set active state
| }) | ||
|
|
||
| const Component = () => { | ||
| const [state, setState] = useState<ActiveState>('idle') |
There was a problem hiding this comment.
Rename to active state or something more descriptive. Just "state" is difficult to understand what means.
| import { BodyText, Button, Feedback, Heading, Loader, Textarea, VerticalSpace } from '@elhub/ds-components' | ||
| import { IconCheckCircle } from '@elhub/ds-icons' | ||
|
|
||
| type ActiveState = 'idle' | 'collecting' | 'submitting' | 'submitted' | 'error' |
There was a problem hiding this comment.
Submitting is not an activeState IMO. Is is a pending state between collecting and submitted, and for that it is better to use the isPending param you get from a fetching library like tanstack query. Not necessary to have an internal "submitting" state.
| const { error: { hasError } = {}, size: formItemSize } = useFormItemContext() | ||
| const [internalVariant, setInternalVariant] = useState<FeedbackVariant | undefined>(defaultVariant) | ||
|
|
||
| const isParentControlled = variant !== undefined |
There was a problem hiding this comment.
I don't understand the purpose of this prop. You have one example using it while another example not using it.
| onClick, | ||
| ...rest | ||
| }) => { | ||
| const { size: contextSize, variant: contextVariant, onVariantChange } = useFeedbackContext() |
There was a problem hiding this comment.
Not sure why the context is needed. I dont think size should be in the context here, since it can and probably should be controlled directly from the consumer app if needed.
The variant prop I'm asking about elsewhere.
There was a problem hiding this comment.
I think a context can be argued for here due to the complexity of the component. The size is still controlled from the consumer size by passing the prop (as it should be)
|
|
||
| const Component = () => { | ||
| const [state, setState] = useState<ActiveState>('idle') | ||
| const [variant, setVariant] = useState<FeedbackVariant | null>(null) |
There was a problem hiding this comment.
You have a variant prop here but its not being used to set the Feedback.Idle.Button variant prop?
But they have the same enum type with "happy", "neutral" and "sad". This is confusing.
| type ActiveState = 'idle' | 'collecting' | 'submitting' | 'submitted' | 'error' | ||
| type FeedbackVariant = 'happy' | 'neutral' | 'sad' | ||
|
|
||
| const wait = (ms: number): Promise<void> => |
There was a problem hiding this comment.
I dont think this function belongs here.
| const submit = async () => { | ||
| if (!variant) return | ||
| setState('submitting') | ||
| await wait(900) |
| } | ||
|
|
||
| const FeedbackFlow: React.FC<FeedbackFlowProps> = ({ title, description, simulateBackendError = false }) => { | ||
| const [feedbackState, setFeedbackState] = useState<ActiveState>('idle') |
There was a problem hiding this comment.
Use activeState and setActiveState here. More descriptive, and the type is ActiveState
| aria-checked={selected} | ||
| className={buttonClass} | ||
| onClick={(event) => { | ||
| onFeedbackTypeChange?.(variant) |
There was a problem hiding this comment.
FeedbackType or Variant? Be consistent and descriptive
3 participants
📝 Description
Add new Feedback component.
Users can rate, comment and submit their feedback anonymously.
Main goal of the component is the keep the bar as low as possible for users to go through with submitting their feedback. The fewer hoops they have to through, the lower risk they will give up before submitting.
🔄 Type of Change
✅ Checklist
📸 Screenshots (if applicable)