feat: migrate role from types #222
Conversation
README.md
Outdated
| @@ -71,6 +72,10 @@ Inject an external JavaScript file. | |||
|
|
|||
| Selectively log team-specific messages to the console. | |||
|
|
|||
| ### [`role`](./src/role.README.md) | |||
|
|
|||
| <!-- Insert summary here --> | |||
There was a problem hiding this comment.
I wasn't able to guess at a useful summary here so suggestions welcome.
There was a problem hiding this comment.
@JamieB-gu Looks like it might have been you that added this initially (guardian/types#33). We're not entirely sure what this is / what it's used for so have you got any suggestions for the summary and a more specific name?
There was a problem hiding this comment.
I can give it a go.
It's a semantic/layout-oriented property that's applied to elements in an article. It describes the "role" of that element in the piece and how it should be laid out within the page. It's probably easiest to understand when thinking about images. Is the image filling a "supporting" role? Is it just a thumbnail (for example the author avatars here)? Is it "showcasing" something (as in this piece). Whilst most commonly used for images, it can also apply to atoms and embeds.
That's the limit of my understanding, but @paperboyo can probably give a more accurate answer with some historical context.
I don't think it has another name; "role" is how it's referred to throughout the CAPI models and most likely in Composer too.
There was a problem hiding this comment.
All what Jamie said is true! Only Composer calls it weighting in the UI, because nothing is ever perfect.
There was a problem hiding this comment.
Great, thanks for explaining that. In that case, could we call it something like ArticleElementRole to make it a bit more explicit? In the meantime, I'll update the README to add this in.
There was a problem hiding this comment.
Awesome, thanks Jamie! 🚀
As an aside: The argument @sndrs made about Format not belonging in libs is perhaps also relevant to Role. If it's specifically an Editorial concept that's going to lose context when placed in a "generic" repo like libs then perhaps this is ultimately not the place for it?
There was a problem hiding this comment.
Yeah, that's a fair point. I've add a comment to the thread in #220 so once we reach a final decision for Format, should we just do the same thing with Role?
src/role.test.ts
Outdated
| import { Role } from './role'; | ||
|
|
||
| it('Role enum contains Standard', () => { | ||
| expect(Role.Standard).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
Similar to #220 (comment), I'm not sure if this is useful beyond keeping coveralls happy.
There was a problem hiding this comment.
At the very least it means that if you’re changing a type, you need to change it in two places, which seems like it’s a good reminded that it is a breaking change?
|
🎉 This PR is included in version 2.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What does this change?
@guardian/typesto@guardian/libsWhy?
See guardian/types#136