-
-
Notifications
You must be signed in to change notification settings - Fork 756
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
References (not just links) to item (view and menu item) – Update to released plone.restapi.relations #4842
Conversation
✅ Deploy Preview for volto ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Passing run #5592 ↗︎Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
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 like where this is headed. I suggested a few things to condense the layout and clean up grammar.
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.
Would it be possible to reduce the image width by resizing the browser width for legibility? Images should be no wider than 740 pixels to fit within the documentation's main view port.
https://6.docs.plone.org/contributing/documentation/authors.html#plone-documentation-styleguide
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.
Of course, thanks for the hint. Changed in c4106b0
# defaultMessage: Content that link to this item | ||
msgid "Content that link to this item" | ||
msgstr "" | ||
|
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 think this message is redundant to the heading above it in the image, Links to "Supervisor Adélaïde Pickavance"
, and I find it confusing. I would remove it.
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.
Me too. It evolved to a state where a description is superfluous.
id="Whenever this item is being referenced from some different item by a hyperlink, block or similar, it appears here in this list." | ||
defaultMessage="Whenever this item is being referenced from some different item by a hyperlink, block or similar, it appears here in this list." | ||
id="Content that link to this item" | ||
defaultMessage="Content that link to this item" | ||
/> | ||
</Segment> |
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 think this is redundant to the heading above it in the table and should be removed.
@@ -27,7 +27,7 @@ Click then on the item labeled {guilabel}`Links to item`. | |||
You can see now a table displaying all links to the current item. | |||
|
|||
```{image} ../_static/user-manual/manage/link-to-items.png | |||
:alt: A panel captioned with "Links to My Summer Vacation - Day 2". A table with two columns (first column labeled with "linked by this item", second column labeled with "review state"). In the row appears a link titled "Links to My Summer Vacation - Day 1", because it is referencing the current item. | |||
:alt: A panel captioned with "Links to My Summer Vacation - Day 2". A table with two columns (first column labeled with "Linking this item", second column labeled with "review state"). In the row appears a link titled "Links to My Summer Vacation - Day 1", because it is referencing the current item. |
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 summarizes the language that I would like to see used and adapted in the code for the panel. I probably missed exactly where the updates would be necessary in the code.
I think the row Content that link to this item
can be merged with the heading above the table, into Content that links to {ITEM_NAME}
.
:alt: A panel captioned with "Links to My Summer Vacation - Day 2". A table with two columns (first column labeled with "Linking this item", second column labeled with "review state"). In the row appears a link titled "Links to My Summer Vacation - Day 1", because it is referencing the current item. | |
:alt: A panel with the heading "Content that links to {ITEM_NAME}", followed by a table describing the content. | |
The table has sections that show the content is linked by either a hyperlink or a reference. | |
The first column is labeled "LINKED BY HYPERLINK" or "LINKED BY REFERENCE '{REF_NAME}'". | |
The second and third columns are labeled "REVIEW STATE" and "TYPE". |
Thank you @stevepiercy for reviewing. I updated the wording according your suggestions and made some more changes. Can you have a look again, please. @pgrunewald and @danalvrz, please take some minutes and share your thoughts, thank you. Preview of docs: https://deploy-preview-4842--volto.netlify.app/user-manual/links-to-item.html |
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.
A spelling correction, a grammar correction (that needs to be applied throughout), and an alt
text that aligns with the new image.
This is good. Once these changes are made, I approve. Thank you!
|
||
```{image} ../_static/user-manual/manage/link-to-items.png | ||
:alt: A panel captioned with "Links to My Summer Vacation - Day 2". A table with two columns (first column labeled with "linked by this item", second column labeled with "review state"). In the row appears a link titled "Links to My Summer Vacation - Day 1", because it is referencing the current item. | ||
:alt: A panel captioned with "Links and references to 'My Summer Vacation - Day 2'". A table with three columns (first column labeled with "Linking / referencing this item…", second column labeled with "Review state", third column labeled with "Type"). In the row appears a link titled "Links to My Summer Vacation - Day 1", because it is referencing the current item. |
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.
:alt: A panel captioned with "Links and references to 'My Summer Vacation - Day 2'". A table with three columns (first column labeled with "Linking / referencing this item…", second column labeled with "Review state", third column labeled with "Type"). In the row appears a link titled "Links to My Summer Vacation - Day 1", because it is referencing the current item. | |
:alt: A panel captioned with "Content that links to or references 'Supervisor Adélaïde Pickavance'". Below the caption, there is a table with three sections, where each section has three columns. The first column's heading is the section name, the second is "Review State", and the third is "Type". The three sections are named, from top to bottom, "Linking this item with hyperlink in text", "Referencing the item as related item", and "Referencing this item with '[Name of Relation Field]'". |
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 applied your suggestion and tried to find wording for the fact that the table is generative: the two relations "hyperlink in text" and "related items" and further more if there are custom relations and items that are relating with these custom relations. Hope that is understandable?
99af3e5
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.
If it is too difficult to describe in words, maybe the table is too complex? Maybe it would be easier to have three tables, instead of three sections in one table? Then we don't have do alt text gymnastics.
I don't know whether mentioning the table is generative would be helpful, and would cause further confusion.
Anyway, when I get stuck on what to do, I usually go with the simpler method. ¯\_(ツ)_/¯
src/components/manage/LinksToItem/__snapshots__/LinksToItem.test.jsx.snap
Outdated
Show resolved
Hide resolved
Co-authored-by: Steve Piercy <web@stevepiercy.com>
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.
LGTM! Thank you!
@ksuess it looks great! 🎉 |
…released plone.restapi.relations (#4842) Co-authored-by: Steve Piercy <web@stevepiercy.com>
Thanks for this great addition, @ksuess! |
PLIP #4339
Update to PR #4787
Nice to have
Made some refactoring: