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

PADV 1584 - Add summarize functionality to plugin #130

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

alexjmpb
Copy link

Description:

This PR is part of the AI assistance project and aims to add the connection to the summarize backend to the annotator js llm_summarize plugin. The plugin sends a request to pearson core summarize api and replaces the annotation text area value with the summary.

How to test:

  • Setup devstack and pull changes.
  • Setup edx notes api service.
  • Select a text inside a text or HTML block.
  • Click on the summarize button.
  • A loader should appear indicating that the content is being summarized.
  • The text area to save the annotation should contain the summarized content.

Screenshots

Screenshot from 2024-09-13 06-56-41

Screenshot from 2024-09-13 06-57-02

Reviewers:

@alexjmpb alexjmpb requested review from kuipumu, Serafin-dev and Squirrel18 and removed request for kuipumu September 13, 2024 12:00
this.modifyDom(this.annotator);
const summarizeButton = document.getElementById('summarizeButton');
summarizeButton.addEventListener('click', function(ev) {
document.querySelector('.annotator-outer.annotator-editor').setAttribute('is_summarizing', true);

Choose a reason for hiding this comment

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

Please, use the same element selector function thought the file, so in case that something in document API changes, we will apply the same change to everything and all will behave in the same way. @alexjmpb

lms/static/js/edxnotes/plugins/llm_summarize.js Outdated Show resolved Hide resolved
editor.fields[1].element.children[0].value = 'ai_summary';
toggleLoader(editor);

const request = new Request("/pearson-core/llm-assistance/api/v0/summarize-text", {

Choose a reason for hiding this comment

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

Please, do not mix single and double quotes. @alexjmpb
Where does Request class come from?

Copy link
Author

Choose a reason for hiding this comment

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

method: 'POST',
headers: {
'Content-Type': 'application/json',
'X-CSRFToken': $.cookie('csrftoken')

Choose a reason for hiding this comment

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

Trailing comma. @alexjmpb

},
body: JSON.stringify({
text_to_summarize: annotation.quote
})

Choose a reason for hiding this comment

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

Trailing comma. @alexjmpb

Comment on lines 136 to 137
if (textArea.style.display === 'none') textArea.style.display = 'block';
else textArea.style.display = 'none';

Choose a reason for hiding this comment

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

Javascript has the toggleAttribute: https://developer.mozilla.org/en-US/docs/Web/API/Element/toggleAttribute ;jQuery has a .toggle function to handle this cases or you can use ternary operator to add the block or none value to the display attribute. @alexjmpb

@@ -12,7 +12,7 @@
$.extend(Annotator.Plugin.LlmSummarize.prototype, new Annotator.Plugin(), {
pluginInit: function() {
// Overrides of annotatorjs HTML/CSS to add summarize button.
var style = document.createElement('style');
let style = document.createElement('style');

Choose a reason for hiding this comment

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

Why is this not a const? @alexjmpb

textAreaWrapper.innerHTML += `
<div class="summarize-loader-wrapper d-none">
<span class="loader" style="
// Style is being defined here since classes styling not working

Choose a reason for hiding this comment

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

Have you used id or class selector? @alexjmpb Please, try to be more specific on the selector, because if this is the case, how are the others styles working?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, even using id it's not applying the styles, I also tried with bs and fa spinners.

@Serafin-dev Serafin-dev changed the base branch from vue/PADV-1580 to pearson-release/olive.stage October 8, 2024 11:54
alexjmpb and others added 2 commits October 8, 2024 09:23
@Serafin-dev Serafin-dev merged commit 785297a into pearson-release/olive.stage Oct 8, 2024
14 of 34 checks passed
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.

3 participants