Add Ask FT button to o-header #1788
Conversation
| header-button-text: oColorsByName('black-80'), | ||
| header-button-icon: oColorsByName('black-80'), | ||
| header-button-background: oColorsByName('black-5'), |
There was a problem hiding this comment.
Added these three variable to generalize colors of a button in the header. Let me know if this is acceptable approach. Alternatively, I can set the same colors directly into the button's scss and keep variables unchanged.
There was a problem hiding this comment.
Hi @Ventzy do these colours apply to just Ask FT buttons or all buttons that could appear in the header?
There was a problem hiding this comment.
It is just for Ask FT button now and we don't plan to add more buttons. If these colors does not look like sensible defaults, then probably would be better to be specified just for the Ask FT button.
There was a problem hiding this comment.
If these colors does not look like sensible defaults, then probably would be better to be specified just for the Ask FT button.
As they're not specific to header buttons. Could they be removed, or alternatively renamed to show that they are colours for ask ft:
header-ask-ft-button-text: ...
header-ask-ft-button-icon: ...
header-ask-ft-button-background: ...
There was a problem hiding this comment.
Renamed as suggested
| @@ -105,3 +105,27 @@ | |||
| } | |||
| // sass-lint:enable no-vendor-prefixes | |||
| } | |||
|
|
|||
| @mixin _oHeaderAskFtButton() { | |||
There was a problem hiding this comment.
Button's main styles. Added as mixing as it is used in both _top.scss and _drawer.scss
There was a problem hiding this comment.
Created tsx/components/ directory as <AskFtButton> component is included in both top.tsx and drawer.tsx. Let me know if there is a better place for it.
|
Thanks for raising this PR :) As there are markup changes, this branch will need to be released as a breaking change. Please ensure there is at least one A migration guide will also need writing. We also have another breaking o-header pull request so please make sure you add your migration guide alongside theirs. |
Ventzy
force-pushed
the
ARP-251-migrate-ask-ft-button-from-dotcom-ui-header-to-o-header
branch
from
be07dd7
to
c852d07
Compare
Ventzy
deleted the
ARP-251-migrate-ask-ft-button-from-dotcom-ui-header-to-o-header
branch
Describe your changes
https://ask.ft.com/ino-header:showAskButtonprop inTHeaderOptionsto control ASK FT button rendering. By default, the button is not renderedshowAskButtonprop ino-headerstoriesAsk FT button was previously added in
dotcom-ui-headerand it is currently active on ft.com home and article pages. This PR adds the same ino-headerand will be followed withdotcom-ui-headerchanged to align witho-header. For reference - this is the PR indotcom-ui-headerwhich has added the button there - Financial-Times/dotcom-page-kit#1031Please review the PR inline comments. I need confirmation if specific approaches taken are fine or adjustments are needed.
Issue ticket number and link
Ticket: https://financialtimes.atlassian.net/browse/ARP-251
Link to Figma designs
https://www.figma.com/design/G0vA3biIFmIeXa0bB3bqWW/ARP%3A-Ask-FT?node-id=6579-37992&t=MyjE38aot5OS5vmH-4
Storybook screenshots
Top
Sticky
Drawer mobile
Checklist before requesting a review
percylabel for o-[COMPONENT] orchromaticlabel for o3-[COMPONENT] on my PR before merging and after review. Find more details in CONTRIBUTING.md