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: Text Generation record card (Align buttons & interaction) #2374

Conversation

leiyre
Copy link
Member

@leiyre leiyre commented Feb 20, 2023

Description

Text Generation record card (Align actions buttons & interaction)

Closes #2335

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested

Checklist

  • I have merged the original branch into my forked branch
  • follows the style guidelines of this project
  • I did a self-review of my code
  • My changes generate no new warnings


<div>
<div class="--editable" v-if="!sentences.length">
<text-2-text-content-editable
Copy link
Contributor

Choose a reason for hiding this comment

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

component name in camelcase

{{ sentence.score | percent }}</base-button
>
</div>
<text-2-text-content-editable
Copy link
Contributor

Choose a reason for hiding this comment

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

component name in camel case

@@ -208,9 +187,13 @@ export default {
},
selectedSentence() {
return (
this.sentences[this.itemNumber] && this.sentences[this.itemNumber].text
this.sentences[this.predictionNumber] &&
this.sentences[this.predictionNumber].text
Copy link
Contributor

@keithCuniah keithCuniah Feb 20, 2023

Choose a reason for hiding this comment

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

replace this.sentences[this.predictionNumber] &&
this.sentences[this.predictionNumber].text by this.sentences[this.predictionNumber]?.text

);
},
isPreannotated() {
return (this.predictionsLength && !this.annotations.length) || false;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

since the left part is always a boolean, the test 'right expression || left expression' can be replace by 'right expression'

@@ -224,54 +207,53 @@ export default {
if (this.sentencesOrigin === "Prediction") {
return this.predictions;
}
if (this.sentencesOrigin === "Preannotation") {
return this.predictions;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

1/replace the 3 if by a swith case
2/ to not have case problem in case this.sentencesOrigin change, compare the same case text

name: "Reset",
allow: this.sentencesOrigin !== "Prediction",
active: this.record.status === "Edited",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

1/duplicate code : add a variable for the 2 test this.sentencesOrigin !== "Prediction" and a function for the test "this.record.status !== "Discarded" or "Edited"
2/ delete comments
3/ create a functionfactory for construc the object { id: "discard", name: "Discard", allow: this.sentencesOrigin !== "Prediction", active: this.record.status !== "Discarded", }

@leiyre leiyre marked this pull request as draft February 20, 2023 16:32
@leiyre leiyre marked this pull request as ready for review February 24, 2023 08:23
const focusInInput = event.target.tagName?.toLowerCase() === "input";
const focusInInput =
event.target.tagName?.toLowerCase() === "input" ||
event.target.contentEditable === "true";
Copy link
Contributor

Choose a reason for hiding this comment

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

add a computed or a method for this equality test

if (this.focus && event.target.isContentEditable) {
event.preventDefault();
var text = event.clipboardData.getData("text/plain");
document.execCommand("insertText", false, text);
Copy link
Contributor

Choose a reason for hiding this comment

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

never use var keyword : const or let. In this case const

set: async function (newValue) {
this.idState.selectedPredictionIndex = newValue;
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

syntactic sugar => in an object, you can replace a function like
get : function() {...} by get() {...}. Same with set

@leiyre leiyre merged commit 0bc2ba7 into feature/bulk-annotation-improvement Feb 28, 2023
@leiyre leiyre deleted the feat/text2text_improvement_and_alignment branch February 28, 2023 11:52
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.

2 participants