Stampy redesign article action buttons #394
Conversation
|
run |
app/components/Article/index.tsx
Outdated
| const actionIds = {helpful: false, unhelpful: false} | ||
|
|
||
| const loadActionTaken = () => { | ||
| forEach(actionIds, (value, key) => { |
There was a problem hiding this comment.
Object.entries(actionIds).forEach((value, key) => {
(...)
})
No need to invoke extra dependancies
app/components/Article/index.tsx
Outdated
| return | ||
| } | ||
| try { | ||
| forEach(actionIds, (value, key) => { |
There was a problem hiding this comment.
why are you always setting all actions, rather than just the current action?
There was a problem hiding this comment.
Because they change values.. an article cannot be helpful and unhelpful simultaneously.
There was a problem hiding this comment.
why not? It absolutely can :D
app/components/Article/index.tsx
Outdated
| } catch (e) {} | ||
| } | ||
|
|
||
| loadActionTaken() |
There was a problem hiding this comment.
this will be run each render - it should be in a useEffect
app/components/Button/button.css
Outdated
| @@ -36,6 +36,7 @@ | |||
| background: var(--colors-white, #fff); | |||
| } | |||
|
|
|||
| .secondary.focused, | |||
| .secondary:hover { | |||
| border: 1px solid var(--colors-teal-200, #a6d9d7) !important; | |||
There was a problem hiding this comment.
it shouldn't change the border - it should change the actual icon. So:
- normal - grey icon
- already clicked - teal icon
This should be possible by setting classes on the actual icon. The old icons hide the coloured SVG paths when they have the focused class
app/components/Article/index.tsx
Outdated
| <ThumbUpIcon /> | ||
| <span className="teal-500">Yes</span> | ||
| </Button> | ||
| <Button className="secondary" action={() => alert('Dislike')}> | ||
| <Button |
There was a problem hiding this comment.
why not just do
import {Action, ActionType} from '~/routes/questions.actions'
(...)
<CompositeButton>
<Action pageid={pageid} showText={false} actionType={ActionType.LIKE} />
<Action pageid={pageid} showText={false} actionType={ActionType.DISLIKE} />
</CompositeButton>
with the appropriate styling/component updates?
There was a problem hiding this comment.
There were mentions of backwards compatibility. "The appropriate styling" would make it unfit on the old domain.
There was a problem hiding this comment.
that's fine. It can live on a separate branch, or even be disabled there
app/components/Article/index.tsx
Outdated
| <ThumbDownIcon /> | ||
| <span className="teal-500">No</span> | ||
| </Button> | ||
| <CompositeButton style={{display: 'flex'}}> |
There was a problem hiding this comment.
don't do inline styles. There is a class for this, so just do className="flex"
app/components/Button/index.tsx
Outdated
| style?: CSSProperties | ||
| } | ||
| export const CompositeButton = ({children, style}: CompositeButtonProps) => ( | ||
| <div className="composite-button" style={style}> |
There was a problem hiding this comment.
yup; classes, not inline styles
app/routes/questions.actions.tsx
Outdated
| </button> | ||
| <Button className={'secondary'} action={() => alert('helpful')}> | ||
| <Icon className={className} /> | ||
| <span className="teal-500"> {showText && title}</span> |
There was a problem hiding this comment.
{showText && (<span className="teal-500"> {title}</span>)}
mruwnik
force-pushed
the
stampy-redesign-articleActionButtons
branch
from
44619b0
to
43b1f1a
Compare
mruwnik
force-pushed
the
stampy-redesign-articleActionButtons
branch
from
43b1f1a
to
381ec2e
Compare
No description provided.