-
Notifications
You must be signed in to change notification settings - Fork 201
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: whole course translations #1330
Conversation
0c48319
to
981c958
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1330 +/- ##
==========================================
- Coverage 88.42% 88.23% -0.20%
==========================================
Files 291 293 +2
Lines 4969 4988 +19
Branches 1263 1266 +3
==========================================
+ Hits 4394 4401 +7
- Misses 561 571 +10
- Partials 14 16 +2 ☔ View full report in Codecov by Sentry. |
3ad314c
to
77fa47d
Compare
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.
Left a couple questions. Since it's top of mind for me; I noticed there's a conflict between paragon versions @edx/paragon and @openedx/paragon.
Solely inputting on the Plugin framework implementation: this looks good! The implementation looks good, including the The main change you'd see is that What ends up taking the place of Note: there is an intention to make the default content more easily customizable when several components are involved (as was the intended feature with defining the content in |
So I added a pr to make the package build correctly openedx/frontend-plugin-framework#48. However, I still run into issue of unavailable context. When I used the transpiled code, the react component that replaced the slot lost the application context. I can't access application data. |
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.
Just one question, but apart from that this looks AWESOME
d2d1744
to
fe6b041
Compare
fe6b041
to
4feff37
Compare
chore: update tests so we have less error message test: update test * test: update tests * chore: remove duplicate translation * chore: lint for console * chore: remove comments * chore: make sure the affect url frame refresh after the language selection change
Add unit tests Fix snapshot Clean Sequence component logEvent calls Clean unit test Put feedback widget behind whole course translation flag Fix useFeedbackWidget test
chore: update plugin instruction
Connect FeedbackWidget with backend services Move feedback widget to unit translation plugin
chore: rewrite and test the api request chore: rebase chore: update translation feedback chore: test chore: add more tests
* fix: feedback widget render error after submit feedback * fix: widget logic
69aef75
to
c1879ea
Compare
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.
Can't thank you enough for adopting FPF and using it for AI translations, @leangseu-edx. Otherwise, beyond it being unfortunate that there are a lot of unrelated changes in this PR (I'd've preferred them in a separate one), there is a big stickler here: we can't have the plugin live in the repository itself. It'll have to be removed from master this week so that it doesn't land in Redwood.
There's also a specifc place (sequence-container
) that will need to be converted to a slot. See comment.
</div> | ||
{enableNewSidebar === 'true' ? <NewSidebar /> : <Sidebar />} | ||
</div> | ||
<div id="whole-course-translation-feedback-widget" /> |
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.
We will likely have to convert sequence-container
to a plugin slot so you can insert this div as part of the plugin. Thoughts?
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 was thinking of doing that as well. It require an extra fetch to the server for the same response. If that is a prefer way, I don't mind.
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.
If a plugin needs to make extra requests, that is not only fine, but expected. I would avoid introducing extra requests to the default experience, though.
To be clear, all I mean to suggest is for sequence-container
to either be wrapped in a <PluginSlot />
or contain one or more of them, not that it should become a plugin itself. In other words, what exists today should at most be "default content".
unitId: PropTypes.string.isRequired, | ||
}; | ||
|
||
export default UnitTranslationPlugin; |
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.
We're going to have to move this plugin out of this repository in the next couple of weeks, before redwood.master
is created.
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.
Should I start the process for make a monorepo or do we have one already?
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.
@leangseu-edx, this plugin can live anywhere at 2U's discretion, with one exception: it can't be in the openedx org. Otherwise, it's up to you folks whether it is even public.
@@ -0,0 +1,24 @@ | |||
import UnitTranslationPlugin from '@plugins/UnitTranslationPlugin'; |
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's great for documentation to have an example plugin defined, but it's unfortunate that we won't be able to leave it pointing to this particular plugin.
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 thought we would leave this as an example.
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.
Sure, but let's at least make it clearly fictitious. Or even better, maybe make a skeleton sample plugin that lives in plugins
, and point to that.
This is not the most urgent bit, though.
@@ -4,6 +4,7 @@ | |||
NODE_ENV='production' | |||
|
|||
ACCESS_TOKEN_COOKIE_NAME='' | |||
AI_TRANSLATIONS_URL='' |
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.
We won't be able to leave this in master
for much longer. See other comments.
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.
This can be move to env.config.js
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.
To 2U's own version of it, yes, but preferably not the example one that lives in this repository.
What changed:
src/
doesn't get much changed. The reason there were a lot of files was because I tried to fix the warning message during tests. It used to print way too many error and warning messages. It was too tedious to debug/write more tests. This doesn't remove all of the warning but it should take care of the common and unnecessary one.