-
Notifications
You must be signed in to change notification settings - Fork 189
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
Implements check to prevent channel having different language from its content #4666
Implements check to prevent channel having different language from its content #4666
Conversation
@@ -174,8 +248,11 @@ | |||
descriptionRequiredMessage: "Please describe what's new in this version before publishing", | |||
descriptionDescriptionTooltip: | |||
'This description will be shown to Kolibri admins before they update channel versions', | |||
languageDescriptionTooltip: 'The default language for a channel and its resources', |
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.
@marcellamaki a heads up on the strings added
cancelButton: 'Cancel', | ||
publishButton: 'Publish', | ||
languageLabel: 'Language', | ||
languageRequiredMessage: 'Please select a language for this channel', |
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.
@marcellamaki a heads up on the strings added
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.
Hi @akolson I've done a code read through and manual QA, and I believe this is working as expected. I'll review the strings with Radina, as we may want to make some adjustments, but we can even do that after merge. I also want to do a final manual QA with a cheffed channel, which I haven't done yet (I expect it should still work! But I want to be thorough).
In the meantime though, the blocker here is for some reason I am seeing a more squished modal than you. I am not sure why it looks so different than your screenshot! I tried in both chrome and firefox and saw this in both browsers.

Thanks @marcellamaki for the review. Strange -- I'll check again. |
@marcellamaki , for some reason i'm still not able to reproduce this, so I am probably missing some context |
Hmmmm well that is certainly interesting 😄 let me investigate further and revert back to you |
@akolson @marcellamaki I tried to replicate the error above but couldn't on Firefox or Chromium (Linux). I did notice that the dropdown is cut off by an overflow:hidden when I open it though. |
@nucleogenesis yes, this one troubled me and didn't find an appropriate fix! Any ideas on how I can fix it? |
I suspect this is connected to previous issues of KModals and things popping out of their parent container (or not popping out, rather) . Not sure if anything mentioned in these past fixes is helpful, Samson. If there's something useful here and there's a relatively quick fix, we can include that. Otherwise, it's okay with me to do this in follow up, as it's not in the publish modal every time but rather an edge case (and it still works). |
@marcellamaki, it seems like KDS v4 doesn't have the fix to the KModal + KSelect issue. And it also looks like KSelect was refactored recently so not sure if the fix was lost in the process? |
I reopened learningequality/kolibri-design-system#324 so we can prioritize it in upcoming KDS releases |
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 @akolson -- let's go ahead and get this merged into unstable, so we will have both the fix and the new strings in. If anything comes up in the QA team's testing similar to what I've seen, we can always open a follow up issue.
Technically, on my side the manual QA confirms the fix (I was not able to test with a cheffed channel that hasn't been published yet), but I'm getting the same badly sized modal as @marcellamaki on both Firefox and Chrome on Windows 10. publish-languages.mp4 |
I suspect that this issue might be related to learningequality/kolibri-design-system#324 |
It looks like the fix is working but in the wrong way! Not sure why its not working on some systems like Jacob's and mine |
I was able to replicate the issue in my local. The bug is as a result of the height calculation of the modal that seems to be a little off and thus the overlap. This will need to be fixed in KDS or furthermore the implementation overhauled using teleportation. For now, we'll set a manual height for the modal body to compensate for the inconsistency and that should fix the issue. cc @marcellamaki |
thank you @akolson! I think that will be a good option for this release. |
Summary
Description of the change(s) you made
This fix ensures that a channel being published has the same language as at least one of the its content. The check is made by providing a dropdown with languages that are present within the channels content. The dropdown is only visible when;
Manual verification steps performed
Screenshots (if applicable)
Sample UI with dropdown

Does this introduce any tech-debt items?
Reviewer guidance
How can a reviewer test these changes?
Are there any risky areas that deserve extra testing?
References
Fixes #3902
Comments
Contributor's Checklist
PR process:
CHANGELOG
label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocs
label has been added if this introduces a change that needs to be updated in the user docs?requirements.txt
files also included in this PRStudio-specifc:
notranslate
class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages
,components
, andlayouts
directories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarn
andpip
)