-
-
Notifications
You must be signed in to change notification settings - Fork 696
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
Show related items #4381
Show related items #4381
Conversation
✅ Deploy Preview for volto canceled.
|
I would say
But this would mean a breaking change, I guess. |
@ksuess I don't think it would be a good idea to do a breaking change for this. Someone who customized the Tags might not like it. I tried to follow the HTML structure of the Semantic UI lists described here: https://semantic-ui.com/elements/list.html Of the React components, I used only the Container to wrap everything. More suggestions? |
@sneridagh @tisto any insight on a visual for this? |
@tiberiuichim I can ask our designer to come up with something. Though, that might take a few days... |
@wesleybl The related functionality is still not present in Volto to the point that it was added as a tutorial block from World Plone Day https://github.com/collective/volto-relateditems-block |
@ichim-david I think related content shouldn't be in blocks. They should appear without explicit user action, as occurs in classic Plone. I will try to update this PR. |
✅ Deploy Preview for plone-components canceled.
|
@ichim-david I have updated this PR. Can you please take a look? |
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.
@wesleybl see my review suggestions
const RelatedItems = ({ relatedItems, intl }) => | ||
relatedItems && relatedItems.length > 0 ? ( | ||
<Container> | ||
<h4 className="ui header">{intl.formatMessage(messages.relatedItems)}</h4> |
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.
The a11y tests will complain that you have a jump from h1 to h4 as it doesn't follow the normal heading ranges from h1-h4.
I suggest an h2 styled to look like the h4
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 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.
Done. I also changed the HTML to look more like
volto-relateditems-block
. Now the Related Items will look like this:I didn't include the CSS because it didn't make a difference.
@wesleybl if this is the final render update the screenshot from the ticket description with this or the latest render.
@wesleybl don't have time to spend on open-source anymore for today, I already do so for too long :) :( but I plan on checking this tomorrow. |
packages/volto/src/components/theme/RelatedItems/RelatedItems.jsx
Outdated
Show resolved
Hide resolved
305497e
to
732213d
Compare
@erral @pnicolli @davisagli @ksuess We need to decide on how we want to display the related items in Volto.
Unless we all chip in on this pull request, if we have any feedback I don't know how we should treat this work because I don't know what is your expectations for the related items viewing especially given that we haven't added it to core until now. |
@ichim-david when I made this PR, slots didn't exist in Volto. But I think it makes sense now for Related Items to be in slots. I think it would be interesting for them to be in the volto/packages/volto/src/components/theme/View/View.jsx Lines 258 to 263 in 8915be3
One detail is that Volto doesn't register anything in the slot. But I don't think there would be a problem registering it, right? Can I make the change? |
@wesleybl I am more in favor of having this functionality as a registration in the below content slot rather than adding more things directly to the template especially since you can even condition the slot addition. Unless we get feedback from the other I don't know in what direction to point you as I don't know what the other core members think about related items. As such I would wait for more feedback before any more development as to not have to make too many changes on and on and on. |
@wesleybl @ichim-david I like the idea of using slots for this, because then it's pretty easy for an add-on to replace the component if it wants to do something different (maybe even different components for different content types). In that case we might not need the |
Yes, I also like the idea of having it in the said slot. If I am not wrong, in Classic UI there is a viewlet to render the related items which is rendered in a viewlet manager. And slots are similar to viewlet manager, si it makes total sense. I also agree with the point that @davisagli raises regarding the need of the Regarding changing the Tags component and adding it to a Slot, it makes total sense, but I think that's out of the scope of this PR. |
Done to Volto 18 |
I only found this issue this week. Yes, putting the related items in the slot is the most flexible and others a good default experience. Please consider setups where you do want authors to control the related items in a block the site could be set up to hide th default slot and add a block for the related items. Both are possible this way. |
@ichim-david Could you update your review please? @plone/volto-team Can we merge this for Volto 18? |
@davisagli I haven't tested the latest changes but everything that is added came out of our comments on what we think it's best. I trust your judgement on how it works with the slot implementation. At this moment I am getting ready for the trip to Ferrara so I am accepting on principle and not due to further testing. EDIT: |
We discussed in the Volto Team meeting. We missed merging this for Volto 18 so we want to merge it with the setting off by default. We can consider turning it on by default in the future in Volto 19. |
@davisagli I changed the default to false. |
@wesleybl Obrigado! |
I need help with the layout:
Can someone help me?
fixes #3740