-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Innovation lab FH-Technikum (wordforms) to master #462
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR! Next to the comments within the code:
There were no changes regarding any dependencies, so there shouldn't be any changes in package.json.lock
and yarn.lock
- please revert them.
@@ -424,9 +424,7 @@ | |||
"printedByAstericsGrid": "Printed by AsTeRICS Grid, https://grid.asterics.eu", | |||
"playingWebradio": "playing: {0}", | |||
"errorPlayingWebradio": "Error playing: {0}, no internet?!", | |||
"webradioVolume": "Webradio Volume: {0} / 100", |
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.
please make sure to only change things related to the changes you've made for the wordforms integration. Maybe also do git merge master
in order to merge the latest changes into your branch and keep changes minimal.
async importFromApi(){ | ||
try { | ||
const word = i18nService.getTranslation(this.gridElement.label); | ||
let response = await fetch('https://wordforms.asterics-foundation.org/wordforms_ndep/scraper.php?verb=' + word + '&type=json'); |
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.
it would be good to move the fetching and parsing of the wordforms to something like wordformFetchService.js
in order to have less logic within this view-oriented element.
const word = i18nService.getTranslation(this.gridElement.label); | ||
let response = await fetch('https://wordforms.asterics-foundation.org/wordforms_ndep/scraper.php?verb=' + word + '&type=json'); | ||
if (!response.ok) { | ||
throw new Error('Network response was not ok'); |
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 intendation of this line is not correct - please auto-format. I think there should be a distinction in the error type/message between (1) network error or (2) an error with the word input.
}); | ||
return wordForm; | ||
}); | ||
this.gridElement.wordForms = wordForms; |
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 the new wordforms should probably be added to the existing ones and not replaced. Like this.gridElement.wordForms = this.gridElement.wordForms.concat(wordForms)
@@ -333,6 +376,17 @@ button { | |||
border-radius: 5px; | |||
} | |||
|
|||
.tooltip { |
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.
please keep the styling of error messages the same like the error message if importing from excel sheets fails.
}, | ||
currentMsg: null, | ||
msgCount: 0, | ||
gridPasteCount: 0 | ||
gridPasteCount: 0, |
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.
superfluous
<div class="row mt-5"> | ||
<label class="col-sm-2" for="inputLabel">{{ $t('label') }}</label> | ||
<div class="col-sm-7"> | ||
<input type="text" class="col-12" id="inputLabel" v-focus v-if="gridElement" v-model="gridElement.label[currentLang]"/> |
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.
v-model="gridElement.label[currentLang]"
is not good, because changing the value of this input value therefore automatically also changes the actual label of the element. Please add a new variable vue instance state like wordFormSearch
, which can be initialized with the current value of the label.
<input type="text" class="col-12" id="inputLabel" v-focus v-if="gridElement" v-model="gridElement.label[currentLang]"/> | ||
</div> | ||
<div class="col-sm-3"> | ||
<button @click="importFromApi()"><i class="fas fa-check"/> {{ $t('retry') }}</button> |
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 the button and search field for the wordform to import should be always visible, not only after an error.
Betreuer: @klues
Wordforms:
Pull request for AsTeRICS-Grid Innolab integration