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

Replace Wikit Textarea with Codex Textarea component #771

Merged
merged 23 commits into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
4dd4db7
Replace Wikit Textarea with Codex Textarea component
guergana Nov 20, 2023
e4e5ff7
Fix v-model and add functionality
guergana Nov 21, 2023
0410cb1
Remove this.form.itemsInput in favor of new v-model syntax
guergana Nov 21, 2023
b62c91c
Fix errors
guergana Nov 21, 2023
9fd7600
Remove trailing comma
guergana Nov 21, 2023
22c83c6
Update browser tests
guergana Nov 21, 2023
9d70229
Update Home.vue
guergana Nov 22, 2023
9aa3703
Update Home.spec.js
guergana Nov 22, 2023
5021f51
Put textarea in a wrapper component
guergana Nov 30, 2023
242bf73
Adjust tests to new TextareaHome component
guergana Nov 30, 2023
6028a8f
Fix linting
guergana Nov 30, 2023
64e9048
Move MAX_NUM_IDS to TextArea component
guergana Dec 4, 2023
54cd3f7
Fix indentation
guergana Dec 4, 2023
97f4f8e
Rename component to ItemIdSearchTextarea
guergana Dec 4, 2023
fcf168c
[WiP] rewrite component using Composition API
guergana Dec 4, 2023
5dffb0c
Commit unsuccesful testing export of i18n :(
guergana Dec 11, 2023
94b4505
Adjust i18n plugin to composition API with useI18n
guergana Dec 13, 2023
fdb98aa
Refactor global variable MAX_NUM_IDS to vue3 format
guergana Dec 13, 2023
ea871ca
Fix typescript errors
guergana Dec 13, 2023
d50dd53
Update HomeState to use the new ValidationError type
guergana Dec 13, 2023
011a249
Add spaces between methods
guergana Dec 14, 2023
35af9ff
Change name of internationalizaton plugin variable
guergana Dec 18, 2023
e6ea197
Remove unneeded options in createI18n plugin
guergana Dec 18, 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 public/i18n/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"about-mismatch-finder-description": "The Mismatch Finder shows you data in Wikidata that differs from the data in another database, catalog or website (for example, someone's date of birth in Wikidata doesn't match the corresponding entry in the German National Library's catalog). Mismatches like this need fixing, and the Mismatch Finder helps you to do just that.",
"find-more": "Find out more",
"item-form-title": "Which Items should be checked?",
"item-form-progress-bar-aria-label": "Progress Bar indicating loading state when submittion Check Items",
"item-form-id-input-label": "Please add one Item identifier per line",
"item-form-id-input-placeholder": "For example:\nQ80378\nQ33602\nQ1459\nQ4524",
"item-form-submit": "Check Items",
Expand Down
1 change: 1 addition & 0 deletions public/i18n/qqq.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"about-mismatch-finder-description": "A short text to describe the Mismatch Finder tool",
"find-more": "A call to action for more information",
"item-form-title": "The title of the form that filters Mismatches by Item id",
"item-form-progress-bar-aria-label": "The aria label for screen reader for the Progress Bar indicating loading state when submittion Check Items",
"item-form-id-input-label": "The label for the Item id input",
"item-form-id-input-placeholder": "The placeholder for the Item id input",
"item-form-submit": "The call to action on the Item id form's submit button",
Expand Down
121 changes: 121 additions & 0 deletions resources/js/Components/TextareaHome.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
<template>
<cdx-field
:status="validationError ? validationError.type : 'default'"
:messages="validationError ? validationError.message : null"
>
<div class="progress-bar-wrapper">
<cdx-progress-bar v-if="loading" :aria-label="$i18n('item-form-progress-bar-aria-label')" />
</div>
<cdx-text-area
:label="$i18n('item-form-id-input-label')"
:placeholder="$i18n('item-form-id-input-placeholder')"
:rows="8"
:status="validationError ? validationError.type : 'default'"
v-model="textareaInputValue"
/>
</cdx-field>
</template>

<script lang="ts">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a new component, I think we should use the composition API instead of the options API already. That is, we can use <script setup lang="ts"> here and not use defineComponent. See:

  1. https://vuejs.org/guide/introduction.html#composition-api
  2. https://vuejs.org/guide/typescript/composition-api.html#typescript-with-composition-api

import { defineComponent, ref } from 'vue';
import { useStore } from '../store';
import { MAX_NUM_IDS } from '../Pages/Home.vue';
guergana marked this conversation as resolved.
Show resolved Hide resolved
import { CdxTextArea, CdxField, CdxProgressBar } from "@wikimedia/codex";

// Run it with compat mode
// https://v3-migration.vuejs.org/breaking-changes/v-model.html
CdxTextArea.compatConfig = {
guergana marked this conversation as resolved.
Show resolved Hide resolved
...CdxTextArea.compatConfig,
COMPONENT_V_MODEL: false,
};

export default defineComponent({
components: {
CdxField,
CdxProgressBar,
CdxTextArea,
},
setup() {
const store = useStore();
Copy link
Member

@itamargiv itamargiv Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we even need to rely on the state store here, if this is to be rally made reusable, we would get the IDs from a prop.

const textareaInputValue = ref(store.lastSearchedIds);

return {
textareaInputValue
};
},
props: {
loading: {
type: Boolean,
default: false
}
},
methods: {
splitInput: function(): Array<string> {
return this.textareaInputValue.split( '\n' );
},
sanitizeArray: function(): Array<string> {
// this filter function removes all falsy values
// see: https://stackoverflow.com/a/281335/1619792
return this.splitInput().filter(x => x);
},
serializeInput: function(): string {
return this.sanitizeArray().join('|');
},
validate(): void {
this.validationError = null;

const typeError = 'error';

const rules = [{
check: (ids: Array<string>) => ids.length < 1,
type: typeError,
message: { [typeError]: this.$i18n('item-form-error-message-empty') }
},
{
check: (ids: Array<string>) => ids.length > MAX_NUM_IDS,
type: 'error',
message: { [typeError]: this.$i18n('item-form-error-message-max', MAX_NUM_IDS) }
},
{
check: (ids: Array<string>) => !ids.every(value => /^[Qq]\d+$/.test( value.trim() )),
type: 'error',
message: { [typeError]: this.$i18n('item-form-error-message-invalid') }
}];

const sanitized = this.sanitizeArray();

for(const {check, type, message} of rules){
if(check(sanitized)){
this.validationError = { type, message };
return;
}
}
},
},
data() {
return {
validationError: null
}
},
});
</script>

<style lang="scss">

.cdx-field__control {
position: relative;
width: 100%;

.progress-bar-wrapper {
position: absolute;
top: 50%;
width: 100%;

.cdx-progress-bar {
width: 50%;
margin: auto;
}
}
}

</style>
96 changes: 34 additions & 62 deletions resources/js/Pages/Home.vue
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,9 @@
</cdx-button>
</div>
<form id="items-form" @submit.prevent="send">
<text-area
:label="$i18n('item-form-id-input-label')"
:placeholder="$i18n('item-form-id-input-placeholder')"
:rows="8"
<textarea-home
Copy link
Member

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 calling the component textarea-home for two reasons.

  1. This name implies and ties the component to be only used in the home page, and that it contains a textarea, both of which are just incidental.
  2. It doesn't actually signify anything about what the component does.

Since the component is in charge of providing a search field for Item IDs, I think a better name for it would be something like item-id-search or something similar that really denotes its purpose.

Copy link
Contributor Author

@guergana guergana Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, wasn't sure what name to give it. Suggestions always come up faster in the review :D Thanks for the name. I still want to make sure it is clear that this is the textarea. I will call it item-id-search-textarea

:loading="loading"
:error="validationError"
v-model="form.itemsInput"
ref="textarea"
/>
<div class="form-buttons">
<cdx-button
Expand All @@ -106,18 +102,15 @@
import { Head as InertiaHead } from '@inertiajs/inertia-vue3';
import { mapState } from 'pinia';
import { useStore } from '../store';
import { TextArea } from '@wmde/wikit-vue-components';
import { CdxDialog, CdxButton, CdxIcon, CdxMessage } from "@wikimedia/codex";
import TextareaHome from '../Components/TextareaHome.vue';
import { cdxIconDie, cdxIconInfo } from '@wikimedia/codex-icons';
import { defineComponent } from 'vue';
import { defineComponent, ref } from 'vue';

interface HomeState {
form: {
itemsInput: string
},
validationError: null|{
type: string,
message: string
message: object
},
faqDialog: boolean
}
Expand All @@ -138,64 +131,31 @@
CdxButton,
CdxIcon,
CdxMessage,
InertiaHead,
TextArea,
TextareaHome,
InertiaHead
},
setup() {
const store = useStore();
const textareaInputValue = ref(store.lastSearchedIds);
guergana marked this conversation as resolved.
Show resolved Hide resolved

return {
cdxIconDie,
cdxIconInfo
cdxIconInfo,
textareaInputValue
};
},
methods: {
splitInput: function(): Array<string> {
return this.form.itemsInput.split( '\n' );
},
sanitizeArray: function(): Array<string> {
// this filter function removes all falsy values
// see: https://stackoverflow.com/a/281335/1619792
return this.splitInput().filter(x => x);
},
serializeInput: function(): string {
return this.sanitizeArray().join('|');
},
validate(): void {
this.validationError = null;

const rules = [{
check: (ids: Array<string>) => ids.length < 1,
type: 'warning',
message: this.$i18n('item-form-error-message-empty')
},
{
check: (ids: Array<string>) => ids.length > MAX_NUM_IDS,
type: 'error',
message: this.$i18n('item-form-error-message-max', MAX_NUM_IDS)
},
{
check: (ids: Array<string>) => !ids.every(value => /^[Qq]\d+$/.test( value.trim() )),
type: 'error',
message: this.$i18n('item-form-error-message-invalid')
}];

const sanitized = this.sanitizeArray();

for(const {check, type, message} of rules){
if(check(sanitized)){
this.validationError = { type, message };
return;
}
}
},
send(): void {
this.validate();
(this.$refs.textarea as InstanceType<typeof TextareaHome>).validate();

if(this.validationError) {
if((this.$refs.textarea as InstanceType<typeof TextareaHome>).validationError) {
return;
}
const store = useStore();
store.saveSearchedIds( this.form.itemsInput );
this.$inertia.get( '/results', { ids: this.serializeInput() } );
store.saveSearchedIds( this.textareaInputValue );
this.$inertia.get( '/results',
{ ids: (this.$refs.textarea as InstanceType<typeof TextareaHome>).serializeInput() }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer it if we didn't directly call component methods. While this can be done with composition API using defineExpose, the best practice here would be to emit an event with the already serialized input and use that event to save them to the component state. Then, this send method becomes much simpler, as it only needs to read the IDs from the state.

);
},
showRandom(): void {
this.$inertia.get( '/random' );
Expand All @@ -216,11 +176,7 @@
...mapState(useStore, ['loading']),
},
data(): HomeState {
const store = useStore();
return {
form: {
itemsInput: store.lastSearchedIds
},
validationError: null,
faqDialog: false
}
Expand Down Expand Up @@ -272,5 +228,21 @@
.form-buttons {
text-align: end;
}

.cdx-field__control {
position: relative;
width: 100%;

.progress-bar-wrapper {
position: absolute;
top: 50%;
width: 100%;

.cdx-progress-bar {
width: 50%;
margin: auto;
}
}
}
}
</style>
4 changes: 2 additions & 2 deletions tests/Browser/ItemsFormTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,15 @@ public function test_can_submit_list_of_item_ids()
});
}

public function test_empty_item_list_yields_warning()
public function test_empty_item_list_yields_error()
{
$this->browse(function (Browser $browser) {
$browser->visit(new HomePage)
->press('.submit-ids')
->assertSee('Please provide the Item identifiers that should be checked.');

$this->assertStringContainsString(
'--warning',
'--error',
$browser->attribute('@items-input-validation-message', 'class')
);
});
Expand Down
2 changes: 1 addition & 1 deletion tests/Browser/Pages/HomePage.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public function elements()
return [
'@form' => '#items-form',
'@items-input' => '@form textarea',
'@items-input-validation-message' => '@form .wikit-TextArea .wikit-ValidationMessage'
'@items-input-validation-message' => '@form .cdx-field__validation-message .cdx-message'
];
}
}
Loading