simorgh
simorgh copied to clipboard
Bylines
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
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 beimport 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
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
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 😄
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 👍
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.