Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

seyfeb
Copy link
Collaborator

@seyfeb seyfeb commented Dec 23, 2023

Topic and Scope

Adds support for the citation property (https://schema.org/citation) to recipe editor and viewer.

Example with citation: "Peter Pan"

Screenshot 2023-12-23 at 18 10 20

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 no citation field?

Formal requirements

There are some formal requirements that should be satisfied. Please mark those by checking the corresponding box.

  • I did check that the app can still be opened and does not throw any browser logs
  • I created tests for newly added PHP code (check this if no PHP changes were made)
  • I updated the OpenAPI specs and added an entry to the API changelog (check if API was not modified)
  • I notified the matrix channel if I introduced an API change

@seyfeb seyfeb added javascript Pull requests that update Javascript code Frontend Issue or PR related to the frontend code labels Dec 23, 2023
Copy link

codecov bot commented Dec 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.07%. Comparing base (116d92f) to head (2a38427).
Report is 507 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2023   +/-   ##
=======================================
  Coverage   80.07%   80.07%           
=======================================
  Files          92       92           
  Lines        2650     2650           
=======================================
  Hits         2122     2122           
  Misses        528      528           
Flag Coverage Δ
integration 21.43% <ø> (ø)
migration 5.69% <ø> (ø)
unittests 57.09% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link

github-actions bot commented Dec 23, 2023

Test Results

   12 files    572 suites   1m 30s ⏱️
  552 tests   552 ✅ 0 💤 0 ❌
2 208 runs  2 207 ✅ 1 💤 0 ❌

Results for commit 2a38427.

♻️ This comment has been updated with latest results.

…cessibility

Signed-off-by: Sebastian Fey <info@sebastianfey.de>
…` for improved accessibility

Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Signed-off-by: Christian Wolf <github@christianwolf.email>
Signed-off-by: Sebastian Fey <info@sebastianfey.de>
Copy link
Collaborator

@christianlupus christianlupus left a 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?

Comment on lines +20 to +24
{{
// TRANSLATORS Indicates citation/source of recipe. Ex. "by Grandma Betty"
t('cookbook', 'by')
}}
<span>{{ $store.state.recipe.citation }}</span>
Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

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']"
Copy link
Collaborator

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.

Copy link
Collaborator Author

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 ;)

Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Issue or PR related to the frontend code javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additional field for source or citation
2 participants