-
Notifications
You must be signed in to change notification settings - Fork 174
Add Feature flag for language-directory #3316
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
base: master
Are you sure you want to change the base?
Add Feature flag for language-directory #3316
Conversation
Yes, the Source Academy frontends should be configurable to provide a desired set of languages and their implementations. The purpose of the language directory is to provide a mapping from language names to the "official" x-slang implementations. What remains to be worked out is to configure what languages should be provided by sourceacademy.org and sourceacademy.nus.edu.sg and other frontends. Should that be done in a config file at build time, or should this also come from a github page? Any proposals? |
@martin-henz best to be done in a config file i think, less layers of indirection especially when setting up new frontends. |
Pull Request Test Coverage Report for Build 17511560243Details
💛 - Coveralls |
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.
minor nits, after resolution LGTM! please make sure the frontend has been tested first.
@@ -41,6 +41,7 @@ | |||
"@reduxjs/toolkit": "^1.9.7", | |||
"@sentry/react": "^10.5.0", | |||
"@sourceacademy/c-slang": "^1.0.21", | |||
"@sourceacademy/language-directory": "https://github.com/loyaltypollution/language-directory.git#866cc934372a19e514af0025947ce7f1f5a6e1fc", |
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.
is it necessary for the language directory to point to your version?
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 seems that his version is newer than the version in SA. Might migrate to SA version after merging.
<ChapterSelectComponent | ||
items={choices.filter(({ mainLanguage }) => mainLanguage === selectedLang)} | ||
onItemSelect={handleChapterSelect} | ||
itemRenderer={chapterRenderer(isFolderModeEnabled)} |
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.
is it right to say that folder mode will be disabled for the new evaluators for now? if so please doc it as a limitation
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.
The folder mode works for new evaluators
src/commons/navigationBar/subcomponents/NavigationBarLangSelectButton.tsx
Show resolved
Hide resolved
src/commons/navigationBar/subcomponents/NavigationBarLangSelectButton.tsx
Outdated
Show resolved
Hide resolved
src/commons/navigationBar/subcomponents/NavigationBarLangSelectButton.tsx
Outdated
Show resolved
Hide resolved
}, [languagesLoaded, dispatch]); | ||
|
||
const directoryEnabled = useFeature(flagLanguageDirectoryEnable); | ||
if (!directoryEnabled) { |
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.
nit: this should be moved higher as a guard clause to cleanly separate logic. to my understanding the code above and below this correspond to the new Navigation bar lang select button, but this corresponds to the original button.
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.
React Hook "useState" is called conditionally. React Hooks must be called in the exact same order in every component render.eslintreact-hooks/rules-of-hooks.
Seems some linting issues if move it higher. So we have to put it here. Correct me if I am wrong.
Simple proof-of-concept for adding additional languages via a
language-directory
.To-Do?
Questions:
-slang
variants support conductors?Happy to collaborate with relevant people on making conductors first-class if that’s the direction we are taking 👍