WSTEAM1-105 Promo refactoring

Open DarioR01 opened this issue 3 years ago • 1 comments

Resolves WSTEAM1-105, WSTEAM1-90, WSTEAM1-94

Overall change:

The first step for the promo refactoring of Promo components in Optimo pages. I have started from the top stories and the related content components, making sure I implemented the new OJ styles described in WSTEAM1-90 and WSTEAM1-94.

Code changes:

In OptimoPromos

In the OptimoPromos folder, I have created all the base components used to construct a Promo. These components should be reusable for future promos. These components would be further styled making sure that the style is global to all promos; e.g. link colours

In Promo

Inside the OptimoPromos Folder, I have created an index.jsx file that works as an entry point to all other elements. The index.jsx file also works as main wrapper for the promo and as ContextProvider to all other Promo elements. Currently we use context to provide:

service of type string
toLink: carries the URL for the link of type string
a11yId: Used to fix a Talkback bug of type string
eventTrackingData: in the shape {componentName: string}
mediaType: The media Type of the promo e.g. video, text, audio, etc... of type string

All these elemental components will go under the name of Promo.ComponentName by following the compound component design pattern, taking advantage of the contextprovider.

In Promo's index.jsx you can further find the following comment: // Currently Outside NewStoryPromo as reusable after which you I reused already existing components which are outside of the OptimoPromos folder. Reused components:

Psammead-image
Psammead-livelabel
timestamp

In PagePromoSection

This is where I have created the specific promos.

Experimenting

Experimenting with Atomic design (removed)

~~By following the Compound component pattern it is easy to also design in an Atomic Design Fashion. The Pros of the Atomic Design Method is that it allows us to organise the code in a deliberate and hierarchical manner.

It also helps us with not building more than one React component in a single file; This is what happened in the case of the TopStories Component where I had to generate a parent index.jsx and smaller molecules to define the Single Promo Item and a List of Promo items.~~ Dropped experimenting.

Experimenting with Compound Components

I found it amazing, I like that the parent Promo can be used as documentation to know what are the building blocks we use for our promos. But Bias aside, what do you think about this methodology. Does this work? Maybe yes but not how I implemented it? Let me know!!!!

Experimenting with LTR/RTL Logic

Nothing much to experiment here. I tried to use padding-inline-start CSS logic in some styles that required it.

To Do

~~Tests~~
~~Click & View Tracking~~
~~LTR/RTL logic~~
~~Using psammead grids instead of display grids (Maybe not needed).~~
~~Related Content Component~~
~~Images Component (possibly reusing the already existing one)~~
~~As @andrewscfc suggested below we could have the PromoAtoms in the component folder, and the Promo could live in the Article Page folder. (Currently all in one folder because I find it convenient 😅 )~~
~~UX Review~~
A11y swarm

A11y related To Do

~~Missing the label on the region that maps to the h2~~
~~Missing role list on the ul~~
~~No faux block link as of yet e.g. the whole promo area isn’t clickable~~
~~Is it possible to have a transparent border on the ‘box’ for each promo with a white background so you can see a border/box edges when colour preferences are changed in FF and in windows high contrast mode? This Border should be 3px wide~~
~~We should only implement a h3 if there is content below in this in the promo e.g. this shouldn’t be a h3 with the current content e.g. no timestamp.~~

Questions

~~Do we like the Compound Component pattern?~~ Seems like we do :D
~~Do we Like the Atomic Design methodology?~~ Seems like we don't
~~How am I going to deal with RTL/LTR? What can I experiment?~~ Tried to follow the draft standard as best as I could.
~~Is it fine to reuse some of these components I reused?~~ Yes, for now, they are not part of the refactoring.

Handover

[ ] Test
[ ] A11y Swarm, all resources here. If testing URL stops working, get the latest storybook deployment in this branch and use these stories:

Make sure to give a link for each:

Single Top Stories
Top Stories with media indicator
Top Stories with Live label
Top Stories with No timestamp
List of Top Stories
Single Related Content
Multiple Related Content
Related Content with no timestamp

Swarm for:

A11y regression on Related Content and Top stories. Help yourself with the past Related Content A11y AC and Screen reader UX.
Promo's with a white card background, A11y AC linked in the Swarm excel.
[ ] To deploy, Delete the extra stories that we used for the a11y swarm (I don't think we need that many stories for our article pages). Then merge to latest

[ ] 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:

[ ] 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

Jun 23 '22 14:06 DarioR01

Great work @DarioR01

Related content
* H2 missing mapping id for the aria-labelledby from the section element

* The wrapping div for the image has a background colour that is the same as the colour already applied on the li, do we need this on the image div as well?

* We should only implement a h3 if there is content below in this in the promo e.g. this shouldn’t be a h3 with the current content e.g. no timestamp
Top stories
* Could we remove the first nested div in the li and apply the CSS that is here to the 2nd nested div that sits there currently? Just a thought, all these divs add to page weight...
h2's Not sure the h2 is the correct font? I also see that the div that wraps this has some CSS such as position relative, z-index which I believe is a left over from the line that we don’t have anymore, not sure we need this? On this point, I wonder if we really need all those nested spans inside the h2 now too? I also see a dir attribute on the inner most span here which doesn’t follow the new standards, also not sure we need to apply a background colour here either? Maybe things to think about when we add the arrow to the section headings?

Bullet Point Ordered Answers: Related Content:

Done
Not My Code, it is used somewhere else and cannot change.
Added to the To Do