Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Add lang attribute to brand component #609

Merged
25 commits merged into from
Jun 17, 2019
Merged

Conversation

EinsteinNjoroge
Copy link
Contributor

@EinsteinNjoroge EinsteinNjoroge commented Jun 11, 2019

Resolves #607

Overall change: Adds a lang attribute ie "en-GB" to ensure brand name ie BBC news is read with the correct accents"

Code changes:

-Wrap "BBC news" text inside a span with a lang attribute with the value set to "en_GB"
-Change the brand components expected props from brandName to product and serviceLocalisedName

Note This calls for changes on the implementation of Brand in Simorgh


  • I have assigned myself to this PR and the corresponding issues
  • Tests added for new features
  • Test engineer approval

@EinsteinNjoroge EinsteinNjoroge self-assigned this Jun 11, 2019
@EinsteinNjoroge EinsteinNjoroge added ws-fp-v1 ws-home Tasks for the WS Home Team labels Jun 11, 2019
@EinsteinNjoroge EinsteinNjoroge requested review from a user and dr3 June 11, 2019 13:16
@EinsteinNjoroge EinsteinNjoroge marked this pull request as ready for review June 11, 2019 13:16
@EinsteinNjoroge EinsteinNjoroge added this to the Brand (WS FP) milestone Jun 11, 2019
@EinsteinNjoroge EinsteinNjoroge requested a review from a user June 12, 2019 10:47
@ghost
Copy link

ghost commented Jun 12, 2019

Don't forget to bump version of the brand component. And also update the changelog

DenisHdz
DenisHdz previously approved these changes Jun 12, 2019
Copy link
Contributor

@DenisHdz DenisHdz left a comment

Choose a reason for hiding this comment

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

Congratulations on your first PR :)

@EinsteinNjoroge EinsteinNjoroge requested a review from a user June 13, 2019 08:35
Copy link
Contributor

@pjlee11 pjlee11 left a comment

Choose a reason for hiding this comment

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

There are references to both Localized and Localised in this PR. I think we should probably use the english Localised in all cases for consistency

pjlee11
pjlee11 previously approved these changes Jun 13, 2019
Copy link
Contributor

@pjlee11 pjlee11 left a comment

Choose a reason for hiding this comment

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

Co-Authored-By: sadickisaac <48688870+sadickisaac@users.noreply.github.com>
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Great work on your first PR.

Copy link
Contributor

@12 12 left a comment

Choose a reason for hiding this comment

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

good

@david-boydell
Copy link

david-boydell commented Jun 14, 2019

Hi @EinsteinCarrey ,

Is this definitely required to be en-GB? In https://www.w3.org/International/questions/qa-html-language-declarations#langvalues it says it should be en.

Cheers
David

@david-boydell
Copy link

I've read that en-GB is also valid. This is ready for merge.

@ghost ghost merged commit a42d662 into latest Jun 17, 2019
@ghost ghost deleted the add-lang-attribute-to-brand-component branch June 17, 2019 08:36
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ws-home Tasks for the WS Home Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add lang attribute to the brand component
7 participants