simorgh icon indicating copy to clipboard operation
simorgh copied to clipboard

Bylines

Open DarioR01 opened this issue 1 year ago • 5 comments

Resolves WSTEAM1-126

NOTE The byline component is in a weird state where new standards are coming out every day. This causes the component to inevitably create technical debt after its creation, or even during its creation. We decided that we will come back and update the component to the latest standards via this
This includes emotion styling, emotion theming, and the usage of basic components such as Text, Paragraph, etc... which are in the process of getting created.

Overall change: Creates a byline component and a new pageData handler

Code changes:

  • Byline complex components using the most recent components standard.
  • Changed the Timestamp logic to only display if there is no byline in ArticlePage.jsx.
  • Creates a pageData handler called handleBylineBlock that removes extra bylines and positions the first byline after the headline.
  • Installed @types/ramda to use ramda library in combination with TypeScript.
  • Adds translation for byline announcements. (Some are missing and will update them once editorial gives them to us)

  • [ ] I have assigned myself to this PR and the corresponding issues
  • [ ] I have added the cross-team label to this PR if it requires visibility across World Service teams
  • [ ] I have assigned this PR to the Simorgh project
  • [ ] (BBC contributors only) This PR follows the repository use guidelines

Testing: Test pages: localhost:7080/arabic/articles/c1er5mjnznzo localhost:7080/pidgin/articles/c1j5mz19jdko

  • [ ] Automated (jest and/or cypress) tests added (for new features) or updated (for existing features)
  • [ ] If necessary, I have run the local E2E non-smoke tests relevant to my changes (CYPRESS_APP_ENV=local CYPRESS_SMOKE=false yarn test:e2e:interactive)
  • [ ] This PR requires manual testing

DarioR01 avatar Aug 12 '22 09:08 DarioR01

Nice one 👍

Main things I can see:

  • There's a new convention around locating components close to where they're used, and only moving them to general locations once they're reused in multiple places. So, I think this component should be colocated with the article page
  • Don't need to import React any more
  • Shouldn't use webpack aliases any more - eg instead of import blah from '#psammead/blah' it should be import blah from '../../legacy/psammead/blah' or something
  • (Eventually, new components shouldn't import stuff from the legacy folder at all, but that's okay for now until the new theming and primitive components are available)
  • This design uses something very similar to the "Curation Subhead" component - https://5d28eb3fe163f6002046d6fa-dmibosjxgz.chromatic.com/?path=/story/topic-curations-subheading--example-with-link - just seems to have a different font size, so may want to generalise that rather than recreating it. Or, at least, make them both use the same SVG

RetiredBlob avatar Aug 16 '22 11:08 RetiredBlob

Nice one 👍

Main things I can see:

* There's a new convention around locating components close to where they're used, and only moving them to general locations once they're reused in multiple places.  So, I think this component should be colocated with the article page

* Don't need to `import React` any more

* Shouldn't use webpack aliases any more - eg instead of `import blah from '#psammead/blah'` it should be `import blah from '../../legacy/psammead/blah'` or something

* (Eventually, new components shouldn't import stuff from the legacy folder at all, but that's okay for now until the new theming and primitive components are available)

* This design uses something very similar to the "Curation Subhead" component - https://5d28eb3fe163f6002046d6fa-dmibosjxgz.chromatic.com/?path=/story/topic-curations-subheading--example-with-link - just seems to have a different font size, so may want to generalise that rather than recreating it.  Or, at least, make them both use the same SVG

'React' refers to a UMD global, but the current file is a module. Consider adding an import instead. Apparently we cannot take away the React import yet, unless we do this

DarioR01 avatar Aug 17 '22 14:08 DarioR01

Looks great, good job @DarioR01 and @mfonofm!

There are new theming options introduced in https://github.com/bbc/simorgh/pull/10230 that may be worth trying here, instead of using the imported colours from Psammead. The fonts will be coming along soon I believe, so it may also be better to wait until then to make use of the theme here. Just something to keep a note of so that we remember to do it 😄

amoore108 avatar Aug 22 '22 12:08 amoore108

The fonts will be coming along soon I believe,

I believe we can slow down the release for now. We are also still waiting for more translations. I will have a look at that soon. Thanks for the review 👍

DarioR01 avatar Aug 22 '22 13:08 DarioR01

All good but think ramda types should be installed as a sep PR so it's easier to trace!

Just off the back of this, could you remove the types/ramda from here @DarioR01 https://github.com/bbc/simorgh/blob/93f1c682640cd59fc612699a81604add60b4bade/package.json#L110

You have the types/ramda package added to devDependencies, which is correct, so the other reference to it in dependencies can be removed.

amoore108 avatar Aug 25 '22 09:08 amoore108