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

pkp/pkp-lib#4787 Reviewer suggestions #426

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

touhidurabir
Copy link
Member

{{ affiliation }}
</div>
<!-- TODO: check alternative of v-html as v-strip-unsafe-html not working -->
<div v-html="suggestionReason"></div>
Copy link
Member Author

Choose a reason for hiding this comment

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

Need better and secure replacement of v-html .

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this resolved?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is related to pkp/pkp-lib#9421 , where we have introduced a new directive v-strip-unsafe-html for 3.3/3.4 but has not yet pushed it for 3.5 . So this is just a placeholder to update it once this change or something similar get pushed for 3.5 .

<!-- TODO: check alternative of v-html as v-strip-unsafe-html not working -->
<div
class="text-sm-normal text-secondary"
v-html="reviewerSuggestion.suggestionReason"
Copy link
Member Author

Choose a reason for hiding this comment

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

Need better and secure replacement of v-html .

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this resolved?

Copy link
Member Author

Choose a reason for hiding this comment

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

same as #426 (comment)

@touhidurabir touhidurabir force-pushed the i4787_main branch 2 times, most recently from 8440d58 to a27ed04 Compare November 29, 2024 09:14
@touhidurabir touhidurabir force-pushed the i4787_main branch 2 times, most recently from ab9e907 to 6453f57 Compare December 24, 2024 19:19
Copy link
Contributor

@defstat defstat left a comment

Choose a reason for hiding this comment

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

Thanks @touhidurabir! Not much here.

Questions:

In the "REVIEWER SUGGESTIONS" there is a drop down control that has a "More Actions" indicator. Should that be a three-dot button doing the same thing?

Image

Also I am wondering whether the newly upgraded TinyMCE affects in any way the forms that are being used in this feature, or rather whether we need to consider changes in this issues PRs regarding the upgraded TinyMCE component.

(actionName) =>
reviewerSuggestionManagerStore[actionName]({
reviewerSuggestion: reviewerSuggestion,
stageAssignmen,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be stageAssignment?

fetchreviewerSuggestion();
});

fetchreviewerSuggestion();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be fetchreviewerSuggestion()?

<!-- TODO: check alternative of v-html as v-strip-unsafe-html not working -->
<div
class="text-sm-normal text-secondary"
v-html="reviewerSuggestion.suggestionReason"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this resolved?

{{ affiliation }}
</div>
<!-- TODO: check alternative of v-html as v-strip-unsafe-html not working -->
<div v-html="suggestionReason"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this resolved?

{{ affiliation }}
</div>
<!-- TODO: check alternative of v-html as v-strip-unsafe-html not working -->
<div v-html="suggestionReason"></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider wrapping long strings like the one bellow?
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we should but I think it should be the part of different task to have a general component to handle such cases instead only for this issue .

@touhidurabir
Copy link
Member Author

Thanks @touhidurabir! Not much here.

Questions:

In the "REVIEWER SUGGESTIONS" there is a drop down control that has a "More Actions" indicator. Should that be a three-dot button doing the same thing?

Image

Also I am wondering whether the newly upgraded TinyMCE affects in any way the forms that are being used in this feature, or rather whether we need to consider changes in this issues PRs regarding the upgraded TinyMCE component.

This has broken due to some new changes in the ui lib , otherwise this is how it should be and was previously
CleanShot 2025-02-04 at 15 29 53@2x

@asmecher
Copy link
Member

asmecher commented Feb 4, 2025

@blesildaramirez, it looks like some TinyMCE changes might be needed here -- can you spot anything quickly that needs adaptation for the new version?

@touhidurabir
Copy link
Member Author

@blesildaramirez, it looks like some TinyMCE changes might be needed here -- can you spot anything quickly that needs adaptation for the new version?

This does not seems like anything related to TinyMce but some changes to the DropdownActions component and the fix is very simple .

@asmecher
Copy link
Member

asmecher commented Feb 5, 2025

This does not seems like anything related to TinyMce but some changes to the DropdownActions component and the fix is very simple .

Thanks, @touhidurabir -- do you mean a simple fix that you can tackle, or that you'll need someone to take care of in ui-library?

@touhidurabir
Copy link
Member Author

This does not seems like anything related to TinyMce but some changes to the DropdownActions component and the fix is very simple .

Thanks, @touhidurabir -- do you mean a simple fix that you can tackle, or that you'll need someone to take care of in ui-library?

I have already fixed it . Working on the PR update based on code review .

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