Ws 2480 implement save button on article page#13932
Ws 2480 implement save button on article page#13932
Conversation
| {isLoading && ( | ||
| <Spinner css={(theme: Theme) => styles.buttonAnimation(theme)} /> | ||
| )} | ||
| {!isLoading && !isSaved && <BookmarkIcon />} |
There was a problem hiding this comment.
Icon is not aligned with text:
There was a problem hiding this comment.
Hm, it doesn't look like this for me.. The icon isn't correct right now, but it is aligned with text for me.
There was a problem hiding this comment.
Same for me as for @elvinasv , alignment and font remain to be adjusted:
| width="13" | ||
| height="18" |
There was a problem hiding this comment.
According to the Figma board, the icon should have been width = 12.5 and height = 17.5. Has this been rounded up? What was decided?
There was a problem hiding this comment.
I think the icon should render at 20x20 to match the Figma design. The 12.5x17.5 is drawn inside the SVG coordinate system.
There was a problem hiding this comment.
Could you please double check spinner and remove icons? Thanks
| backgroundColor: isSaved ? palette.WHITE : palette.WHITE, | ||
| color: isSaved ? palette.GREY_8 : palette.GREY_8, |
There was a problem hiding this comment.
I think we can remove isSaved if the colors are consistent between states
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| height: '2.75rem', | ||
| fontFamily: 'ReithSans, Arial, Helvetica, sans-serif', |
There was a problem hiding this comment.
The font depends on the service used. Let's use this approach:
| height: '1rem', | ||
| width: '1rem', | ||
| animation: `${spinAnimation} 1s linear 0s infinite normal none running`, | ||
| animationName: spinAnimation, |
There was a problem hiding this comment.
I think animationName is redundant because it's already included in the animation shorthand.
| @@ -0,0 +1,74 @@ | |||
| import { css, Theme, keyframes } from '@emotion/react'; | |||
|
|
|||
| const spinAnimation = keyframes` | |||
There was a problem hiding this comment.
Minor: we could use object syntax, which would work better with linting. I.e.
const spinAnimation = keyframes({
'0%': { transform: 'rotate(0deg)' },
'100%': { transform: 'rotate(360deg)' },
});| outline: `3px solid ${palette.GREY_8}`, | ||
| boxShadow: `0 0 0 ${palette.WHITE}, 0 0 0 9px ${palette.GREY_8}`, |
There was a problem hiding this comment.
Should we use pixelsToRem instead of px values here?
| onClick={onClick} | ||
| disabled={disabled || isLoading} | ||
| aria-label={label} | ||
| title={label} |
There was a problem hiding this comment.
Out of curiosity: do we expect the button title? Was this suggested by the UX?
There was a problem hiding this comment.
Good catch, this wasn't in the UX spec. Removing it.
| // return null; | ||
| } | ||
|
|
||
| const buttonLabel = isSaved |
There was a problem hiding this comment.
We shouldn't have default values here. If translations are not defined, we could do an early return. That should help with TS errors.
| disabled={isLoading} | ||
| label={buttonLabel} | ||
| buttonText={buttonText} | ||
| removeText={saveArticleButton?.removeButton ?? 'Remove'} |
There was a problem hiding this comment.
Same as above - there shouldn't be hardcoded translations
| closeLabel: 'Close', | ||
| buttonSeparatorText: 'या', | ||
| }, | ||
| saveArticleButton: { |
There was a problem hiding this comment.
Minor: button suffix is redundant.
saveArticleButton: {
save: 'Save for later',
saving: 'Saving',
saved: 'Saved to My News',
remove: 'Remove',
}
| width="13" | ||
| height="18" |
There was a problem hiding this comment.
Could you please double check spinner and remove icons? Thanks
Resolves JIRA: https://bbc.atlassian.net/browse/WS-2480
Summary
Introducing ActionButton and refactoring existing SaveArticleButton into a HOC/container
Default:
On hover:
On focus:
Saving:
Saved:
Saved on hover:
Code changes
Testing
Useful Links
Figma design: https://www.figma.com/design/O5es2iJ7Ocjm85He1wc38q/WS-Save-for-later-Component?node-id=256-10697&t=7NGD3io8UIDF6DRG-0