-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stampy redesign 368 #412
Stampy redesign 368 #412
Changes from 7 commits
413d86d
922388b
34d98c2
f4bfbda
9664932
d907169
4073ce8
3f16217
dcba680
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
.feedback-container { | ||
width: 384px; | ||
padding: var(--spacing-32); | ||
} | ||
.select-option { | ||
height: var(--spacing-48); | ||
padding: var(--spacing-8) var(--spacing-16); | ||
margin: var(--spacing-8) 0; | ||
cursor: pointer; | ||
width: 100%; | ||
} | ||
.select-option.selected { | ||
border: 1px solid var(--colors-teal-500); | ||
} | ||
|
||
.feedback-form { | ||
width: 100%; | ||
position: relative; | ||
text-align: left; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't text-align left by default? If not we should define this in the body tag, so we don't have to define it again here |
||
font-size: 16px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want to ever write CSS related to typography, just reference whichever class was assigned to it in Figma! |
||
color: var(--colors-cool-grey-600); | ||
z-index: 2; | ||
} | ||
|
||
.feedback-text { | ||
width: calc(100% - var(--spacing-24)); | ||
max-width: calc(100% - var(--spacing-24)); | ||
height: 128px; | ||
padding: var(--spacing-12); | ||
margin: var(--spacing-40) 0; | ||
box-sizing: content-box; | ||
} | ||
.feedback-text.no-options { | ||
margin: var(--spacing-16) 0; | ||
} | ||
.submit-feedback.enabled { | ||
cursor: pointer; | ||
opacity: 1; | ||
} | ||
|
||
.tooltip:hover::after { | ||
display: block; | ||
} | ||
|
||
.action-feedback-text { | ||
display: none; | ||
position: absolute; | ||
left: 180px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm skeptical about how these kinds of things will convert to mobile but let's see what happens |
||
text-wrap: nowrap; | ||
top: 13px; | ||
} | ||
.action-feedback-text.show { | ||
display: block; | ||
} | ||
.composite-button > .feedback-form { | ||
position: absolute; | ||
left: 90px; | ||
top: -342px; | ||
display: none; | ||
} | ||
.composite-button > .feedback-form.show { | ||
display: block; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
import React, {ChangeEvent} from 'react' | ||
import Button from '~/components/Button' | ||
import './feedbackForm.css' | ||
|
||
export interface FeedbackFormProps { | ||
/** | ||
* Article ID | ||
*/ | ||
pageid: string | ||
/** | ||
* Class name | ||
*/ | ||
className?: string | ||
/** | ||
* onBlur | ||
*/ | ||
onBlur?: () => void | ||
/** | ||
* onFocus | ||
*/ | ||
onFocus?: () => void | ||
/** | ||
* Has Options | ||
*/ | ||
hasOptions?: boolean | ||
} | ||
const FeedbackForm = ({ | ||
pageid, | ||
className = 'feedback-form', | ||
onBlur, | ||
onFocus, | ||
hasOptions = true, | ||
}: FeedbackFormProps) => { | ||
// to be implemented. | ||
console.log(pageid) | ||
const [selected, setSelected] = React.useState<string>() | ||
const options = [ | ||
{ | ||
text: 'Making things up', | ||
option: 'making_things_up', | ||
}, | ||
{ | ||
text: 'Being mean', | ||
option: 'being_mean', | ||
}, | ||
{ | ||
text: 'Typos', | ||
option: 'typos', | ||
}, | ||
] | ||
const [enabledSubmit, setEnabledSubmit] = React.useState(false) | ||
const selectFeedback = (option: string) => { | ||
setSelected(option) | ||
|
||
if (onFocus) { | ||
onFocus() | ||
} | ||
setEnabledSubmit(true) | ||
} | ||
const handleBlur = React.useCallback( | ||
(e: ChangeEvent<HTMLElement>) => { | ||
const currentTarget = e.currentTarget | ||
|
||
// Give browser time to focus the next element | ||
requestAnimationFrame(() => { | ||
// Check if the new focused element is a child of the original container | ||
if (!currentTarget.contains(document.activeElement)) { | ||
if (onBlur) { | ||
onBlur() | ||
} | ||
} | ||
}) | ||
}, | ||
[onBlur] | ||
) | ||
|
||
const handleSubmit = () => {} | ||
|
||
return ( | ||
<div className={className} onBlur={handleBlur} onFocus={onFocus}> | ||
<div className={'feedback-container bordered'}> | ||
<span className={'black'}>What was the problem?</span> | ||
{hasOptions | ||
? options.map((option, index) => ( | ||
<Button | ||
key={index} | ||
className={[ | ||
option.text == selected ? 'secondary-alt selected' : 'secondary', | ||
'select-option', | ||
].join(' ')} | ||
action={() => selectFeedback(option.text)} | ||
> | ||
<div>{option.text}</div> | ||
</Button> | ||
)) | ||
: null} | ||
|
||
<textarea | ||
name={'feedback-text'} | ||
className={['feedback-text bordered', !hasOptions ? 'no-options' : ''].join(' ')} | ||
placeholder={'Leave a comment (optional)'} | ||
></textarea> | ||
<Button className="primary full-width" action={handleSubmit} disabled={!enabledSubmit}> | ||
<p>Submit feedback</p> | ||
</Button> | ||
</div> | ||
</div> | ||
) | ||
} | ||
|
||
export default FeedbackForm |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import {useState} from 'react' | ||
import React, {useState} from 'react' | ||
import {Link} from '@remix-run/react' | ||
import KeepGoing from '~/components/Article/KeepGoing' | ||
import CopyIcon from '~/components/icons-generated/Copy' | ||
|
@@ -8,17 +8,33 @@ import {Action, ActionType} from '~/routes/questions.actions' | |
import type {Glossary, Question} from '~/server-utils/stampy' | ||
import Contents from './Contents' | ||
import './article.css' | ||
import FeedbackForm from '~/components/Article/FeedbackForm' | ||
|
||
const isLoading = ({text}: Question) => !text || text === 'Loading...' | ||
|
||
const ArticleFooter = (question: Question) => { | ||
const [showFeedback, setShowFeedback] = useState(false) | ||
const [showFeedbackForm, setShowFeedbackForm] = useState(false) | ||
const [isFormFocused, setIsFormFocused] = useState(false) | ||
const date = | ||
question.updatedAt && | ||
new Date(question.updatedAt).toLocaleDateString('en-GB', { | ||
year: 'numeric', | ||
month: 'short', | ||
}) | ||
|
||
React.useEffect(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not move all this stuff into the feedback form component? It's only hidden, so the HTML elements will still be around There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for storybook functionality There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's the wrong way round. Storybook, tests etc. shouldn't change how the actual code is structured. If something is hard to do without changes, then it's a hint that something is wrong, but the code should dictate storybook, not the other way round There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can add extra wrappers with decorators - check the ArticlesKeepGoing story There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been looking into this but I don't believe it's a feasible option. The article makes it display when submitted, moving part of the logic to the Feedback component would just duplicate logic and code. |
||
// Hide the form after 10 seconds if the user hasn't interacted with it | ||
const timeoutId = setInterval(() => { | ||
if (!isFormFocused) { | ||
setShowFeedbackForm(false) | ||
} | ||
}, 10000) | ||
|
||
// Clear the timeout to prevent it from running if the component unmounts | ||
return () => clearInterval(timeoutId) | ||
}, [showFeedbackForm, isFormFocused]) | ||
|
||
return ( | ||
!isLoading(question) && ( | ||
<div className="footer-comtainer"> | ||
|
@@ -35,8 +51,28 @@ const ArticleFooter = (question: Question) => { | |
<span>Did this page help you?</span> | ||
|
||
<CompositeButton className="flex-container"> | ||
<Action pageid={question.pageid} showText={true} actionType={ActionType.HELPFUL} /> | ||
<Action pageid={question.pageid} showText={true} actionType={ActionType.UNHELPFUL} /> | ||
<Action | ||
pageid={question.pageid} | ||
showText={true} | ||
actionType={ActionType.HELPFUL} | ||
onSuccess={() => setShowFeedback(true)} | ||
/> | ||
<Action | ||
pageid={question.pageid} | ||
showText={true} | ||
actionType={ActionType.UNHELPFUL} | ||
onSuccess={() => setShowFeedbackForm(true)} | ||
/> | ||
<span className={['action-feedback-text', showFeedback ? 'show' : ''].join(' ')}> | ||
Thanks for your feedback! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @melissasamworth what will cause this to be hidden again? Should it disappear e.g. after 10s? Otherwise it will hang around and be annoying There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that works, disappears after 10s There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this also should probably go into the feedback form |
||
</span> | ||
<FeedbackForm | ||
pageid={question.pageid} | ||
className={['feedback-form', showFeedbackForm ? 'show' : ''].join(' ')} | ||
onBlur={() => setIsFormFocused(false)} | ||
onFocus={() => setIsFormFocused(true)} | ||
hasOptions={false} | ||
/> | ||
</CompositeButton> | ||
</div> | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,7 +72,7 @@ | |
border-right: 1px solid transparent; | ||
} | ||
|
||
.composite-button > form:last-child .button, | ||
.composite-button > form:last-of-type .button, | ||
.composite-button > .button:last-child { | ||
border-top-right-radius: 6px; | ||
border-bottom-right-radius: 6px; | ||
|
@@ -104,6 +104,13 @@ | |
z-index: 1; | ||
} | ||
|
||
.tooltip:hover::after { | ||
display: block; | ||
.button.full-width { | ||
width: 100%; | ||
height: 48px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say use the button component and apply the relevant class |
||
border: 0; | ||
} | ||
.button:disabled, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these kinds of things should be added to the button component |
||
.button[disabled] { | ||
opacity: 0.6; | ||
cursor: inherit; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,19 +8,29 @@ type ButtonProps = { | |
className?: string | ||
tooltip?: string | ||
icon?: ReactNode | ||
disabled?: boolean | ||
} | ||
const Button = ({children, action, tooltip, icon, className}: ButtonProps) => { | ||
const Button = ({children, action, tooltip, icon, className, disabled = false}: ButtonProps) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how would this work for link buttons? You can add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it necessary for anything else besides the feedback form? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not now, but it would be good to have it done for consistency |
||
const classes = ['button', className, tooltip && 'tooltip'].filter((i) => i).join(' ') | ||
if (typeof action === 'string') { | ||
return ( | ||
<Link to={action} className={classes} data-tooltip={tooltip}> | ||
<Link | ||
to={action} | ||
className={classes} | ||
data-tooltip={tooltip} | ||
onClick={(e) => { | ||
if (disabled) { | ||
e.preventDefault() | ||
} | ||
}} | ||
> | ||
{children} | ||
{icon && icon} | ||
</Link> | ||
) | ||
} | ||
return ( | ||
<button className={classes} onClick={action} data-tooltip={tooltip}> | ||
<button className={classes} onClick={action} data-tooltip={tooltip} disabled={disabled}> | ||
{children} | ||
</button> | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import type {Meta, StoryObj} from '@storybook/react' | ||
import FeedbackForm from '../app/components/Article/FeedbackForm' | ||
const meta = { | ||
title: 'Components/Article/FeedbackForm', | ||
component: FeedbackForm, | ||
tags: ['autodocs'], | ||
} satisfies Meta<typeof FeedbackForm> | ||
export default meta | ||
type Story = StoryObj<typeof FeedbackForm> | ||
export const Default: Story = { | ||
args: { | ||
pageid: 'NH50', | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is feedback container? We'll want to be added .col-* class to different elements instead of defining the width through writing more CSS. I imagine this might be one of those cases? :)