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.

Sign up for GitHub

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

Feat/507 add citation field #2023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2036cd4
fix: Add aria-labelledby property to `EditInputField` for improved ac…
seyfeb Dec 23, 2023
8bc6005
fix: Add placeholder and aria-placeholder property to `EditInputField…
seyfeb Dec 23, 2023
01b87ab
feat: Add `citation` field to recipe editor
seyfeb Dec 23, 2023
2fd7790
feat: Add `citation` field to `RecipeView`
seyfeb Dec 23, 2023
720924b
chore: Update CHANGELOG
seyfeb Dec 23, 2023
2a38427
style: Fix code style
seyfeb Dec 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### Added
- Toast with success/error message after trying to copy ingredients [#2040](https://github.com/nextcloud/cookbook/pull/2040) @seyfeb
- Seconds can now be specified for recipe times [#2014](https://github.com/nextcloud/cookbook/pull/2014) @seyfeb
- Added support for citation field [#2023](https://github.com/nextcloud/cookbook/pull/2023) @seyfeb

### Fixed
- Prevent yield calculation for ## as ingredient headline [#1998](https://github.com/nextcloud/cookbook/pull/1998) @j0hannesr0th
Expand Down
18 changes: 17 additions & 1 deletion src/components/FormComponents/EditInputField.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<template>
<fieldset>
<label>
<label :id="inputLabelId">
{{ fieldLabel }}
</label>
<textarea
Expand All @@ -9,6 +9,9 @@
"
ref="inputField"
v-model="content"
:placeholder="placeholder"
:aria-labelledby="inputLabelId"
:aria-placeholder="placeholder"
@input="handleInput"
@keydown="keyDown"
@keyup="handleSuggestionsPopupKeyUp"
Expand All @@ -23,6 +26,9 @@
ref="inputField"
v-model="content"
:type="props.fieldType"
:placeholder="placeholder"
:aria-labelledby="inputLabelId"
:aria-placeholder="placeholder"
@input="handleInput"
@keydown="keyDown"
@keyup="handleSuggestionsPopupKeyUp"
Expand Down Expand Up @@ -64,6 +70,10 @@ const props = defineProps({
default: false,
required: false,
},
placeholder: {
type: String,
default: '',
},
suggestionOptions: {
type: Array,
default: () => [],
Expand All @@ -89,6 +99,12 @@ const suggestionsData = ref(null);
*/
const content = ref(props.value);

/**
* Unique identifier used for identifying the input label for accessibility reasons.
* @type {import('vue').Ref<string>}
*/
const inputLabelId = ref(`input-label-${Math.floor(Math.random() * 10000000)}`);

// deconstruct composable
const {
suggestionsPopupVisible,
Expand Down
11 changes: 11 additions & 0 deletions src/components/RecipeEdit.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

:field-type="'text'"
:field-label="t('cookbook', 'Source')"
:placeholder="
// TRANSLATORS Example (placeholder) name for a citation of the recipe's source
t('cookbook', 'Grandma Betty')
"
/>
<EditInputField
v-model="recipe['url']"
:field-type="'url'"
Expand Down Expand Up @@ -201,6 +210,7 @@ const recipe = ref({
id: 0,
name: '',
description: '',
citation: '',
url: '',
image: '',
prepTime: '',
Expand Down Expand Up @@ -531,6 +541,7 @@ const initEmptyRecipe = () => {
id: 0,
name: null,
description: '',
citation: '',
url: '',
image: '',
prepTime: '',
Expand Down
22 changes: 20 additions & 2 deletions src/components/RecipeView/RecipeView.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,18 @@
</div>

<div class="meta">
<h2 class="heading">{{ $store.state.recipe.name }}</h2>
<div class="heading">
<h2 class="mb-0">{{ $store.state.recipe.name }}</h2>
<p v-if="$store.state.recipe.citation">
{{
// TRANSLATORS Indicates citation/source of recipe. Ex. "by Grandma Betty"
t('cookbook', 'by')
}}
<span>{{ $store.state.recipe.citation }}</span>
Comment on lines +20 to +24
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.

</p>
</div>
<div class="details">
<div v-if="recipe.keywords.length">
<div v-if="recipe.keywords.length" class="mb-3">
<ul v-if="recipe.keywords.length">
<RecipeKeyword
v-for="(keyword, idx) in recipe.keywords"
Expand Down Expand Up @@ -816,6 +825,14 @@ export default {
</script>

<style lang="scss" scoped>
.mb-0 {
margin-bottom: 0 !important;
}

.mb-3 {
margin-bottom: 0.75rem !important;
}

.wrapper {
width: 100%;
}
Expand Down Expand Up @@ -873,6 +890,7 @@ export default {

.heading {
margin-top: 12px;
margin-bottom: 1rem;
}

.dates {
Expand Down
Loading