Skip to content
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

WORLDSERVICE 408 - Implement Lite site text CTA under h1 on article page (Gahuza) #12494

Merged
merged 94 commits into from
Mar 25, 2025

Conversation

Louis-Matsika
Copy link
Contributor

@Louis-Matsika Louis-Matsika commented Mar 11, 2025

Resolves JIRA WORLDSERVICE-408

Overall changes

Created a new component CallToActionLinkWithChevron within #components

This new component uses the existing CallToActionLink however incorporates <RightChevron> and <LeftChevron> (to support both LTR, and RTL languages)

Created ArticleHeadline co-located within src/app/pages/ArticlePage. this component handels rendering heading on article pages and decides if a headline should include a LiteSiteCTA based on the toggles set in iSite

Notes

Refactoring
There are now a few CTAs within Simorgh that are logically and visually similar. A ticket will be made to address refactoring

insuring there isn’t multiple LiteSiteCTAs on an article page
Currently these changes affect every heading on an article page (this should be fine as there shouldn't be more than 1 h1 heading on an article page).

if work is needed to only apply the CTA under the first heading(as there may be more headings on the page), then work in the BFF would need to be done. perhaps adding a custom block that apply's CallToActionLink only to the first heading in the data that gets sent off to Simorgh

Testing

Canonical Articles that should display the liteSiteCTA

Screenshot 2025-03-24 at 3 34 19 PM
  1. Visit any article page on a service with the liteSiteCTA toggle enabled

    • e.g. http://localhost:7080/gahuza/articles/c5y51yxeg53o
  2. Check if the liteSite CTA is visible under the Article heading

  3. Click the CTA

    The CTA should take you to the same article however the lite version

    • e.g. http://localhost:7080/gahuza/articles/c5y51yxeg53o.ltie

Canonical Articles that should not display the liteSiteCTA

Screenshot 2025-03-24 at 3 37 42 PM
  1. Visit any article page on a service with the liteSiteCTA toggle disabled
    • e.g. http://localhost:7080/gahuza/articles/c5y51yxeg53o

The liteSite CTA should not be visible under the Article heading

The LiteSite

The LiteSite itself should not render the liteSiteCTA under any conditions
Screenshot 2025-03-19 at 2 26 24 PM

  1. Visit any article page on the LiteSite
    • e.g. http://localhost:7080/gahuza/articles/c5y51yxeg53o

The liteSite CTA should not be visible under the Article heading

Helpful Links

Coding Standards

Repository use guidelines

@Louis-Matsika Louis-Matsika self-assigned this Mar 11, 2025
@Louis-Matsika Louis-Matsika added the enhancement New feature or request label Mar 11, 2025
@greenc05
Copy link

Thanks @Isabella-Mitchell So we also have future work to refactor the call to action link (without chevron)? (Not sure we have any other components in this state)

@Isabella-Mitchell
Copy link
Contributor

Isabella-Mitchell commented Mar 19, 2025

Ok so now I've seen the component on the page and see the touch target height is good. Wondering why though that isn't in the component when it's viewed in isolatoin in storybook?
Would be great if we could start some best practice for PRs (@Isabella-Mitchell does an outstanding job of this), in which we can include links to components in isolatoin and in page please @karinathomasbbc @amoore108 @HarveyPeachey

Touch target could be related to this comment: #12494 (comment)

Seems like it needs the header in place for the touch target to be correct, whereas I think we should have all the styling encapsulated within the CTA component itself.

We've added display:inline-block to the link styles. This means the is not treated as inline, and so the touch target spacing is shown on Storybook on the link in isolation.. Agree it's something we could look more into during refactoring.

We've added some more stories to help us see the component in context
LTR
RTL
Heading with no Lite Site CTA

@Isabella-Mitchell
Copy link
Contributor

Thanks @Isabella-Mitchell So we also have future work to refactor the call to action link (without chevron)? (Not sure we have any other components in this state)

We'll put together a ticket for this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be overkill to update the ArticlePage stories to include an example with liteSiteCTA enabled? This is probably better for being able to see the component in context. We could even consider removing this story, as we don't really need to see the article headline in isolation...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okies, that's my bad. I thought it would be helpful to have co-located stories for reviewing. But agree the best reviewing experience would be to see it on the Article Page. And then we don't need any new fixture data to do this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, it was useful to have initially, I just thought it was best to view the component in context & it made sense on the Article Page

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make a couple of changes to the stories?

Here's what I would like:

  1. A generic story (similar to the example) where I can select the service and the page furniture will automatically change (even though the article content will remain the same, we should still get translations for the selected service). This should be named ArticlePageWithLiteSiteCTA but allow service to be passed in as a prop. This should also have chromatic snapshots disabled
  2. A story for chromatic (which could be the current ArticlePageWithLiteSiteCTA story with gahuza pre-selected, but renamed to TestArticlePageWithLiteSiteCTA), which is actually hidden from view in Storybook - here's how we've done this elsewhere:
    // This story is for chromatic testing purposes only
    export const Test = {
    render: (_: StoryArgs, { variant }: StoryProps) => (
    <Component service="kyrgyz" variant={variant} />
    ),
    tags: ['!dev'],
    };
  3. Delete ArticlePageWithLiteSiteCTARightToLeft because this will be superseded by 1.

Happy to hop on a call to discuss.

@karinathomasbbc karinathomasbbc merged commit 8661159 into latest Mar 25, 2025
11 checks passed
@karinathomasbbc karinathomasbbc deleted the 444-lite-site-cta branch March 25, 2025 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants