-
Notifications
You must be signed in to change notification settings - Fork 92
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
Feat/507 add citation field #2023
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2023 +/- ##
=======================================
Coverage 80.07% 80.07%
=======================================
Files 92 92
Lines 2650 2650
=======================================
Hits 2122 2122
Misses 528 528
Flags with carried forward coverage won't be shown. Click here to find out more. |
Test Results 12 files 572 suites 1m 30s ⏱️ Results for commit 2a38427. ♻️ This comment has been updated with latest results. |
…cessibility Signed-off-by: Sebastian Fey <[email protected]>
…` for improved accessibility Signed-off-by: Sebastian Fey <[email protected]>
Signed-off-by: Sebastian Fey <[email protected]>
Signed-off-by: Sebastian Fey <[email protected]>
Signed-off-by: Sebastian Fey <[email protected]> Signed-off-by: Christian Wolf <[email protected]>
Signed-off-by: Sebastian Fey <[email protected]>
2918372
to
2a38427
Compare
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 have some concerns. ne can be quickly fixed. The other needs some more discussion, I guess?
{{ | ||
// TRANSLATORS Indicates citation/source of recipe. Ex. "by Grandma Betty" | ||
t('cookbook', 'by') | ||
}} | ||
<span>{{ $store.state.recipe.citation }}</span> |
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 is not a good way to handle this wrt i10n. In other languages, it might not be in the pattern static text plus name. So, this might be better delegated to the translators.
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.
Strictly speaking, you are right. I made this decision to allow different styles for the word "by" and the source string, which might even become an HTML element later on, like
<p>by <a :href="source-uri">Peter Pan</a></p>
But it would be better to translate the whole string including the citation variable, then HTML-escaping the citation, string-replacing the citation in the translated string with the html-escaped one (plus any additional styling, linking, etc) and using v-html in a span. Maybe there is a simpler method that I don't know of?
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 about something like 'by %s%s%s'
with the three parts <a href="...">
, $store.state.recipe.citation
, and </a>
? In the first part, we would only have to insert the href
value which can be handled manually in this case.
Theoretically, one could fall back to by %s
and do the concatenation in JS directly.
@@ -16,6 +16,15 @@ | |||
:field-label="t('cookbook', 'Description')" | |||
:suggestion-options="allRecipeOptions" | |||
/> | |||
<EditInputField | |||
v-model="recipe['citation']" |
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 am not so sure about this usage of the citation
field. According to the schema.org standard, This field seems inappropriate according to the definition of this field:
A citation or reference to another creative work, such as another publication, web page, scholarly article, etc.
This does not match the use case, especially with the Grandma Betty placeholder content. How about using author
or the creator
fied.
I find it hard to quickly merge this PR now and soon have to create some sort of migration/fix that changes the code a posteriori.
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 "not so sure" part is true for the other fields to. So thank you for initiating the discussion!
According to schema.org the author
and creator
fields are the same
creator
: The creator/author of this CreativeWork. This is the same as the Author property for CreativeWork.
However, I tend towards the creator being the one that creates the recipe in the cookbook, i.e., the Nextcloud user (analog to the fact that the dateCreated
field is the non-user-configurable date the recipe was created, and not the date when grandma invented the recipe). This could become relevant when real sharing of cookbooks, recipes, collections, ... between users becomes a thing. It would allow filtering recipes by the user who entered the recipe in the cookbook app. As usual, we could store this info outside of the JSON ...
On the other hand, what I, as a creator, am doing when creating the recipe is citing "My grandma", who told me about the list of ingredients and steps to take, but not necessarily the exact wording of each instruction, or the exact format for entering ingredients. I also might add some more comments on how grandma used to watch birds in the front yard while stirring the sauce but still cite her because she originally had the idea.
I do see the point though, that when I want to cite
The Science of Cooking - Every Question Answered to Perfect Your Cooking,
Stuart Farrimond, Dorling Kindersley Limited, 2017
the approach with "by XXX" falls a bit short and the creator
(or author
) field would make it much clearer what to expect from it, i.e., the "by XXX" syntax could be expected.
One option would be to get rid of the word "by" and leave it up to the user to format the string to be shown using Markdown, which would also immediately allow linking. However my idea of moving the url
property to a href of the citation to clean up the UI when viewing a recipe would then be off the table. I think, it's the usual consideration configurability vs. cleanness/ease of use.
Wrapping up, I see how both options have advantages and disadvantages. I'm looking forward for your opinion ;)
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 general problem I see is the distinction in schema.org between a natural person/organization and a third (creative) work. The former is referencable by author
/creator
, while the latter is referenced by citation
. In my understanding, these associations are mutually exclusive in some sort.
How about the following idea: We could have both fields (citation
and author
). One could be used as a source (in the sense of a reference), that would be a citation
, while Grandma's best recipes could be declared using author
. Would that work?
Of course, the UI must be thought through here as well. We would have 4 cases (no field, either field is set, or both fields are set). If none is set, we are at status quo. With one field set, your approach might work well (by author or from citation for example). With both present, we could think about it but I would then go with by author and add a reference later on like the URL source currently.
Topic and Scope
Adds support for the
citation
property (https://schema.org/citation) to recipe editor and viewer.Example with
citation: "Peter Pan"
Closes #507
Concerns/issues
Possible UI/UX option: Make citation text an anchor if
URL
property is set. This would make for a cleaner look (no URL is shown). Caveats: What if URL is set, but nocitation
field?Formal requirements
There are some formal requirements that should be satisfied. Please mark those by checking the corresponding box.