-
Notifications
You must be signed in to change notification settings - Fork 9
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
WIP: Listing variations by content type #462
base: main
Are you sure you want to change the base?
Conversation
{item?.head_title && <div className="headline">{item.head_title}</div>} | ||
<HeadingTag className="title"> | ||
{item.title ? item.title : item.id} | ||
</HeadingTag> |
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.
This keeps the same tag that was used before (h2 or h3) for backwards compatibility, but ideally we would not use those tags, since this isn't part of the page outline (table of contents).
const { item, HeadingTag = 'h3' } = props; | ||
return ( | ||
<> | ||
{item?.head_title && <div className="headline">{item.head_title}</div>} |
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.
I really wish we could switch this class to use kicker
instead of headline
, especially since headline
is also used for the heading at the top of the listing block.
); | ||
}; | ||
|
||
export default NewsItemSummary; |
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.
There's quite a bit repeated between the default summary and the custom ones for News Item and Event -- when really we are only changing the kicker. Should we be adding a Kicker component instead?
On the other hand, doing it this way makes it possible to show something entirely different than the title + description if that makes more sense for some content type.
config.getComponent({ | ||
name: 'Summary', | ||
dependencies: [item['@type']], | ||
}).component || DefaultSummary; |
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.
Does using a component in the registry just make it harder to explain than updating an object in config.settings (i.e. like contentTypeViews)?
year: 'numeric', | ||
month: 'long', | ||
day: 'numeric', | ||
}); |
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 ought to update the FormattedDate component in volto to use this instead of momentjs, but that'll have to be in a major version...
Opening a draft PR so I can put notes on things I want to discuss...
What's included here:
To do: