-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Timeline model #245
Timeline model #245
Conversation
@davidfurey has published a preview version of this PR with release workflow run #60, based on commit 17f5ef3: 23.0.0-PREVIEW.timeline-model.2024-03-26T1038.17f5ef3f Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the timeline-model branch, or use the GitHub CLI command: gh workflow run release.yml --ref timeline-model Want to make a full release after this PR is merged?Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command: gh workflow run release.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks consistent with our requirements 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes all look reasonable. Thrift being the authority for so much of our modelling, I do feel like a lot of what's here would benefit from more description, especially when so many modelling decisions we make are quite tactical.
I've added some comments asking questions about what are likely perfectly reasonable modelling decisions, and I wonder if in answering them you could add those comments to the model for future reference.
struct TimelineEvent { | ||
1: required list<BlockElement> body = []; | ||
|
||
2: optional BlockElement main; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between a main element and the body elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the main
block (used for main media) and body
blocks (used for the article), we will allow a single main element and then have the rest of the content in body. This will make it easier for the platforms to render the first image differently.
|
||
4: optional string date; | ||
|
||
5: optional string label; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this label used? We already have a title, but it looks like there's additional description required, I wonder whether we can document why here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
3: optional string title; | ||
|
||
4: optional string date; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presume the reason we are not using CapiDateTime
here is because this is audience facing and may be formatted in many different ways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to allow timelines to include events where the date is not exactly known, where it could be a range, etc. At some point in the future I'd like to explore making this machine readable (perhaps using https://www.loc.gov/standards/datetime/). But for version 1, a string seems reasonable.
What does this change?
How to test
How can we measure success?
Have we considered potential risks?
Images
Accessibility