-
Notifications
You must be signed in to change notification settings - Fork 699
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
Fix coreStrings import in AttemptLogList #12429
Fix coreStrings import in AttemptLogList #12429
Conversation
Build Artifacts
|
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.
Thanks @AlexVelezLl - I'm requesting changes to block merge until @rtibbles has a chance to look at this if he's able to tomorrow. He and I discussed this a bit and I recall he was skeptical of the need to import the file differently here than in other parts of the code.
One thing I noticed is that this appears to be an issue with the core
package itself in that all plugins can import import coreStrings...
but the one file in core
cannot.
kolibri/plugins/user_auth/assets/src/views/LoginSideNavEntry.js|4 col 1| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/plugins/user_profile/assets/src/views/UserProfileSideNavEntry.js|4 col 1| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/plugins/learn/assets/src/views/LearnSideNavEntry.js|3 col 1| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/plugins/learn/assets/src/views/LibraryPage/OtherLibraries.vue|145 col 3| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/plugins/learn/assets/src/views/ExamPage/AnswerHistory.vue|109 col 3| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/plugins/learn/assets/src/composables/useLearningActivities.js|11 col 1| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/plugins/learn/assets/src/my_downloads/views/MyDownloadsSideNavEntry.js|3 col 1| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/plugins/device/assets/src/views/DeviceManagementSideNavEntry.js|6 col 1| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/plugins/facility/assets/src/views/FacilityManagementSideNavEntry.js|4 col 1| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/plugins/coach/assets/src/csv/fields.js|4 col 1| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/plugins/coach/assets/src/views/CoachSideNavEntry.js|3 col 1| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/plugins/coach/assets/src/views/common.js|2 col 1| import coreStrings from 'kolibri.utils.coreStrings';
kolibri/core/assets/src/views/AttemptLogList.vue|157 col 3| import coreStrings from 'kolibri.utils.coreStrings';
This may be a quirk of our build system and it may be one that can be fixed on the build-system level but that's probably more work than is necessary in light of our timeline.
I'm keen to merge this and make a follow-up in either case. Let's see if Richard is back tomorrow and has any thoughts as well, otherwise we'll merge this and make a follow-up
LGTM as well, though there's a 2024-07-10_16-26-21.mp4 |
Thank you @pcenov! I have fixed this |
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.
Thanks @AlexVelezLl!
Richard gave it a look, the full fix for the issue will be done fixing #5488, but this can be merged in the meantime
Summary
For some webpack import issues, when importing coreStrings from a component inside core using
kolibri.utils.coreStrings
for some reason this import gets the default import of the filecommonCoreString
which is a mixin, instead of importing the named export of the coreStrings Translator. Referencing directly using relative import is a workaround to solve this e.g..References
Closes #12424.
Reviewer guidance
Replicate the escenario showed in ##12424.
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)