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

Spell Checker Interface [WIP] #253

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

Conversation

JPJPJPOPOP
Copy link
Contributor

@JPJPJPOPOP JPJPJPOPOP commented Jan 5, 2018

content[splitWords[originalWordsIndex]] = '<div class="list-group">';
for(var sugg = 0; sugg < data[tokenIndex]['sugg'].length; sugg++) {
content[splitWords[originalWordsIndex]] += '<a href="#" class="list-group-item">' +
data[tokenIndex]['sugg'][sugg][0] + '</a>';

Choose a reason for hiding this comment

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

["sugg"] is better written in dot notation dot-notation

$('#spellcheckerInput').html($('#spellcheckerInput').html() + ' <span class="spellError" id=' +
splitWords[originalWordsIndex] + '>' + splitWords[originalWordsIndex] + '</span>');
content[splitWords[originalWordsIndex]] = '<div class="list-group">';
for(var sugg = 0; sugg < data[tokenIndex]['sugg'].length; sugg++) {

Choose a reason for hiding this comment

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

["sugg"] is better written in dot notation dot-notation

success: function (data) {
var originalWordsIndex = 0;
for(var tokenIndex = 0; tokenIndex < data.length; tokenIndex++) {
if(data[tokenIndex]['known'] === true) {

Choose a reason for hiding this comment

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

["known"] is better written in dot notation dot-notation

@JPJPJPOPOP JPJPJPOPOP force-pushed the spellchecker branch 5 times, most recently from a2ea8f0 to 7583760 Compare January 6, 2018 04:54
placement: 'bottom',
trigger: 'manual',
html: true,
content: content[currentTokenId]

Choose a reason for hiding this comment

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

'content' is not defined no-undef

for(var sugg = 0; sugg < data[tokenIndex].sugg.length; sugg++) {
content[splitWords[originalWordsIndex]] += '<a href="#" class="spellCheckerListItem">' +
data[tokenIndex].sugg[sugg][0] + '</a>';
content[splitWords[originalWordsIndex]] += '</div>';

Choose a reason for hiding this comment

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

'content' is not defined no-undef
'splitWords' is not defined no-undef

splitWords[originalWordsIndex] + '>' + splitWords[originalWordsIndex] + '</span>');
content[splitWords[originalWordsIndex]] = '<div class="spellCheckerList">';
for(var sugg = 0; sugg < data[tokenIndex].sugg.length; sugg++) {
content[splitWords[originalWordsIndex]] += '<a href="#" class="spellCheckerListItem">' +

Choose a reason for hiding this comment

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

'content' is not defined no-undef
'splitWords' is not defined no-undef

}
$('#spellCheckerInput').html($('#spellCheckerInput').html() + ' <span class="spellError" id=' +
splitWords[originalWordsIndex] + '>' + splitWords[originalWordsIndex] + '</span>');
content[splitWords[originalWordsIndex]] = '<div class="spellCheckerList">';

Choose a reason for hiding this comment

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

'content' is not defined no-undef
'splitWords' is not defined no-undef

continue;
}
$('#spellCheckerInput').html($('#spellCheckerInput').html() + ' <span class="spellError" id=' +
splitWords[originalWordsIndex] + '>' + splitWords[originalWordsIndex] + '</span>');

Choose a reason for hiding this comment

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

'splitWords' is not defined no-undef

var originalWordsIndex = 0;
for(var tokenIndex = 0; tokenIndex < data.length; tokenIndex++) {
if(data[tokenIndex].known === true) {
$('#spellCheckerInput').html($('#spellCheckerInput').html() + ' ' + splitWords[originalWordsIndex]);

Choose a reason for hiding this comment

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

'splitWords' is not defined no-undef

@JPJPJPOPOP JPJPJPOPOP force-pushed the spellchecker branch 2 times, most recently from c52e3ae to 02ab612 Compare January 6, 2018 06:06
@JPJPJPOPOP JPJPJPOPOP changed the title Spellchecker Interface [WIP] Spell Checker Interface [WIP] Jan 6, 2018
@JPJPJPOPOP JPJPJPOPOP force-pushed the spellchecker branch 2 times, most recently from b5960d5 to 1f1165c Compare January 6, 2018 06:22
@apertium apertium deleted a comment from houndci-bot Jan 6, 2018
@apertium apertium deleted a comment from houndci-bot Jan 6, 2018
Copy link
Member

@sushain97 sushain97 left a comment

Choose a reason for hiding this comment

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

Initial run through, looking on the right track. Haven't tested it yet though.

I don't have too much time right now so I couldn't take a more detailed look.

Makefile Outdated
@@ -46,7 +46,8 @@ JSFILES= \
assets/js/translator.js \
assets/js/analyzer.js \
assets/js/generator.js \
assets/js/sandbox.js
assets/js/sandbox.js \
assets/js/spellchecker.js
Copy link
Member

Choose a reason for hiding this comment

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

Spacing

Copy link
Contributor Author

@JPJPJPOPOP JPJPJPOPOP Jan 6, 2018

Choose a reason for hiding this comment

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

@sushain97 this is weird, it looks fine in the actual file, but looks wrong here.

@@ -13,7 +13,8 @@ var localizedLanguageCodes /*: {[string]: string} */ = {}, localizedLanguageName

/* global config, getPairs, getGenerators, getAnalyzers, persistChoices, getURLParam, cache, ajaxSend, ajaxComplete, sendEvent,
srcLangs, dstLangs, generators, analyzers, readCache, modeEnabled, populateTranslationList, populateGeneratorList,
populateAnalyzerList, analyzerData, generatorData, curSrcLang, curDstLang, restoreChoices, refreshLangList, onlyUnique */
populateAnalyzerList, analyzerData, generatorData, curSrcLang, curDstLang, restoreChoices, refreshLangList, onlyUnique
Copy link
Member

Choose a reason for hiding this comment

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

Missing a comma?

'translation': getPairs,
'generation': getGenerators,
'analyzation': getAnalyzers,
'spellchecker': getSpellers
Copy link
Member

Choose a reason for hiding this comment

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

Is it just me or is something off, shouldn't it be 'spell checking'? Otherwise, 'translation' would be 'translator', etc.

Copy link
Member

Choose a reason for hiding this comment

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

(and in other analagous places)

index.html.in Outdated
</div>
<div class="col-sm-4">
<select class="form-control spellCheckerMode" id="secondarySpellCheckerMode" name="secondarySpellCheckerMode">
<option> </option>
Copy link
Member

Choose a reason for hiding this comment

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

Spacing

}

function spellCheckerNotAvailable(data) {
$('#spellCheckerInput').append($('<div></div>').text(' '));
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of .text(' ')?

@sushain97
Copy link
Member

sushain97 commented Jan 6, 2018

Also, please add the flow annotation to spellchecker.js (which should probably be called spellchecking.js)?

@sushain97
Copy link
Member

sushain97 commented Jan 6, 2018 via email

@JPJPJPOPOP
Copy link
Contributor Author

@sushain97 I don't think it should be called spellchecking.js, because there are already files in the assets/js like translator.js, generator.js, etc.

@JPJPJPOPOP JPJPJPOPOP force-pushed the spellchecker branch 12 times, most recently from 939d8b6 to f3537ab Compare January 7, 2018 00:00
@sushain97
Copy link
Member

It looks like there are two dropdowns here:

image

However, only tat provides a speller:

$ curl http://localhost:2737/list?q=spellers
{"oci": "oci-spell", "crh": "crh-spell", "kaz": "kaz-spell", "kir": "kir-spell", "hin": "hin-spell", "tat": "tat-spell", "tyv": "tyv-spell"}

What gives?

@sushain97
Copy link
Member

Also, please undo your test data commit since I got this up with real data.

var currentSpellCheckerRequest;

/* exported getSpellers, populateSecondarySpellCheckerList */
/* global config, modeEnabled, persistChoices, readCache, ajaxSend, ajaxComplete, filterLangPairList, allowedLang, analyzers, cache,

Choose a reason for hiding this comment

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

'analyzers' is defined but never used no-unused-vars

@sushain97
Copy link
Member

Couple concerns:

image

language name selector should have localized names? they're correct in other places

image
seems to be glitchy if the API doesn't provide any suggestions?

deferred.resolve();
}
});*/
var data = {"eng":"test1"};

Choose a reason for hiding this comment

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

Strings must use singlequote quotes
Missing space before value for key 'eng' key-spacing

if(modeEnabled('spellchecking')) {
populatePrimarySpellCheckerList(spellerData);
}

Choose a reason for hiding this comment

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

Trailing spaces not allowed no-trailing-spaces

@@ -304,6 +310,10 @@ function localizeLanguageNames(localizedNamesFromJSON) {
if(modeEnabled('analyzation')) {
populateAnalyzerList(analyzerData);
}
if(modeEnabled('spellchecking')) {
populatePrimarySpellCheckerList(spellerData);
}

Choose a reason for hiding this comment

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

Trailing spaces not allowed no-trailing-spaces

@@ -13,7 +13,8 @@ var localizedLanguageCodes /*: {[string]: string} */ = {}, localizedLanguageName

/* global config, getPairs, getGenerators, getAnalyzers, persistChoices, getURLParam, cache, ajaxSend, ajaxComplete, sendEvent,
srcLangs, dstLangs, generators, analyzers, readCache, modeEnabled, populateTranslationList, populateGeneratorList,
populateAnalyzerList, analyzerData, generatorData, curSrcLang, curDstLang, restoreChoices, refreshLangList, onlyUnique */
populateAnalyzerList, populatePrimarySpellCheckerList, analyzerData, generatorData, spellerData, curSrcLang, curDstLang, restoreChoices, refreshLangList, onlyUnique,

Choose a reason for hiding this comment

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

Line 16 exceeds the maximum line length of 140 max-len

}
$('.spellError').each(function () {
var currentTokenId = this.id;
if(content[currentTokenId].indexOf("spellCheckerListItem") !== -1) {

Choose a reason for hiding this comment

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

Strings must use singlequote quotes

@JPJPJPOPOP JPJPJPOPOP force-pushed the spellchecker branch 3 times, most recently from d501dc1 to 649fb8c Compare January 9, 2018 04:10
@apertium apertium deleted a comment from houndci-bot Jan 9, 2018
@@ -13,7 +13,8 @@ var URL_PARAM_Q_LIMIT = 1300,
'#translation': 'q',
'#webpageTranslation': 'qP',
'#analyzation': 'qA',
'#generation': 'qG'
'#generation': 'qG',
'#spellchecking': 'qS'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sushain97 This currently conflicts with sandbox. What should I change this to?

Copy link
Member

Choose a reason for hiding this comment

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

I don't really think sandbox needs one...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sushain97 It at least needs something, otherwise errors start popping up and breaks the UI.

Copy link
Member

Choose a reason for hiding this comment

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

Or let's just make it qSand.

@STSARC001
Copy link

hello everyone i am A frontend Devolper And i Want to be part of Your Community In This GSOC 2023
i Will Clear this Spell Checker Issue With the Help of API

Name :- Abhijit Dengale
email:- [email protected]

@jonorthwash @xavivars

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants