-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 part of #19570: language name in language dropdown list in contributor admin dashboard #19594
Conversation
Hi @Aakash-Jakhmola, can you complete the following:
|
Assigning @nikitaevg for the first pass review of this PR. Thanks! |
PTAL @nikitaevg |
@Aakash-Jakhmola Hi! how are you? About this PR, the correction is to put the written languages in the list in English first? If it's because the pr hasn't been approved yet, I'm sorry. So I see that from the image it is correct. |
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 @Aakash-Jakhmola -- just a few comments.
@@ -189,6 +189,21 @@ export class ContributorAdminDashboardPageComponent implements OnInit { | |||
); | |||
} | |||
|
|||
rearrangeLanguageName(language: string): string { |
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.
A more concrete description might be putEnglishLanguageNameAtFront()
rearrangeLanguageName(language: string): string { | ||
// Check if the language contains the expected format 'abc (something)'. | ||
if (language.includes('(') && language.includes(')')) { | ||
// Split the language into parts using '(' and ')' as separators |
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.
Instead of doing all this could you just construct a regex for the desired format and check whether the string matches it? You can use capture groups to collect the relevant parts. I think this would lead to cleaner code and less string manipulation.
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.
@seanlip I am using RegExp(/([\p{L}]+)\s*\(([\p{L}]+)\)/, 'u')
but getting lint error.
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.
Please provide the error you see, it's difficult to help without understanding the error you get.
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.
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.
@Aakash-Jakhmola The error message says bad escape \p at position 2
but I'm not sure what's causing that. It seems to work fine when I try it here ... https://esprima.org/demo/validate.html
https://bugs.eclipse.org/bugs/show_bug.cgi?id=492025 suggests that correctness is determine by the "runtime's RegExp impl" but I'm not sure what that's referring to exactly.
I wonder if the issue is that whatever impl is being used doesn't recognize the \p{L}. Could you try replacing that with "accept any character" and see if that helps? Also btw the second part of the regex should just be [A-Za-z ], right?
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.
Done.
* fix oppia#19511: make name optional for subscribing * fix lint * use Optional * return non_empty validator * remove Union
…oppia#19402) * Fix oppia#18661: specific partnership form link * linting * frontend tests * ts-checks * fixes * minor changes * resolve conflict * fix tests
changes done in about-page.component.css Co-authored-by: 2008sahil <getinter6@gmail.com>
* Fix Reset Button Styling on Graph * Change to Flow Root
* added fixed height * fixed search bar * lint --------- Co-authored-by: Nikita Vorobev <vorobyev.nikita@gmail.com>
…19483) * Trap command used to check for Sudden Exit of running tests on code * trap command is added with sigint handler * trap command is added with sigint handler * trap command is added with sigint handler * trap command is added with sigint handler * trap command is added with sigint handler * changed done as reuqired * coe updated to check for exit aprt from keyboard interrupt also * code for all tests updated in makeflie to stop containers in any case of failure * code for all tests updated in makeflie to stop containers in any case of failure * make stop will executed for all exit code apart from 0 now * removed extra spaces * removed the extra echo commands * backend tests workflow changed * backend tests workflow changed * removed the extra echo commands
…when selecting old ones. (oppia#19549) * Fix Exploration Editor Category Select * Change to Value Change
oppia#19565) * button replaced by primary-button in android page, also disabled attribute added * fix linter error
* Creates voiceover model * Adds test for entity voiceover model * Add voiceover domain and service methods. * Adds voiceover domain and services tests * Fixes mypy checks * Creates voiceover policy model * Fixes linter issues. * Address review comments * Adds regex check for language accent code * Updates voiceover policy model name. * Adds voiceover language accent constants * Adds language accent codes list for voiceovers * updates docstring * updates docstring * Deletes voice policy dir and shifts it into voiceover dir * Adds helper methods to get & save in VoiceoverAutogenerationPolicyModel * Creates json files to stores L-a constants. * Adds test to validate voiceover language accent constants. * Removes dead code * Updates code comment * Updates code comment * Updates Readme. * Adds note to developers * Updates code comment
…9122) * Front-end changes * Fix frontend tests * Fix frontend tests * Fix linting * Fix tests * Fix tests * Fix tests * Fix linting * Fix linting * Fix linting * Fix CI checks * Fix all linting errors * Update the frontend files and write unit tests * Fix failing unit tests * Fix failing unit tests * Fix failing unit tests * Fix failing unit tests * Fix failing CI checks * Fix failing unit tests * Fix CI Checks * Fix lint checks * Fix typescript typecheck errors * Fix typescript typecheck errors * Fix typescript typecheck errors * Address Review Comments * Fix flaky tests * Fix lints and add coverage tests * Fix lints and add coverage tests
…cellation (oppia#19579) Fix Customization Args Saved Memento
…pia#19588) searched results alligned to center Co-authored-by: 2008sahil <getinter6@gmail.com>
for (const language of component.languageChoices) { | ||
if (language.id === 'hi-en') { | ||
expect(language.language).toBe('Hinglish'); | ||
} else if (language.id === 'ms') { | ||
expect(language.language).toBe('Bahasa Melayu (بهاس ملايو)'); | ||
} else if (language.id === 'az') { | ||
expect(language.language).toBe('Azerbaijani (Azeri)'); | ||
} | ||
} |
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.
Generally speaking, if-else statements shouldn't really be used in top-level component tests. It might be better to create a Jasmine spy on AppConstants.SUPPORTED_AUDIO_LANGUAGES
that returns our initial test data before arrangement.
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.
@StephenYu2018 I am not sure how to do this.
I tried this.
spyOnProperty(AppConstants, 'SUPPORTED_AUDIO_LANGUAGES')
.and.returnValue([
{
id: 'ms',
description: 'بهاس ملايو(Bahasa Melayu)',
relatedLanguages: ['ms'],
direction: 'ltr'
},
]);
But it is not working, it gives this error.
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 think maybe what you can do instead is have something along the lines of "assert that languageChoices contains these three dicts". E.g. expect(languageChoices.some(condition)).toBeTrue()
where condition checks that the dict matches a target dict.
(Not sure if list.includes() would work, but that would be even better/simpler.)
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.
Done. Used toInclude
component.selectLanguage('Arabic (العربية)'); | ||
expect(component.selectedLanguage.language).toBe('Arabic (العربية)'); |
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 you also do the same thing here as you did in lines 173-180 within this file?
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.
Done.
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.
A few more comments to address before merging.
Unassigning @StephenYu2018 since the review is done. |
Hi @Aakash-Jakhmola, it looks like some changes were requested on this pull request by @StephenYu2018. PTAL. Thanks! |
Done with changes. PTAL @StephenYu2018. Thanks! |
Unassigning @Aakash-Jakhmola since a re-review was requested. @Aakash-Jakhmola, please make sure you have addressed all review comments. Thanks! |
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.
LGTM!
Unassigning @StephenYu2018 since they have already approved the PR. |
Hi @Aakash-Jakhmola, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
Hi @Aakash-Jakhmola. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
Head branch was pushed to by a user without write access
…ontributor admin dashboard (oppia#19594) * fix language name in language dropdown to list language name first * change var to let * fix integration test * Fix oppia#19511: make name optional for subscribing (oppia#19534) * fix oppia#19511: make name optional for subscribing * fix lint * use Optional * return non_empty validator * remove Union * Fix oppia#18661: Specific partnership interest form for each language (oppia#19402) * Fix oppia#18661: specific partnership form link * linting * frontend tests * ts-checks * fixes * minor changes * resolve conflict * fix tests * Fix issue oppia#19408 About Page is not responsive (oppia#19530) changes done in about-page.component.css Co-authored-by: 2008sahil <getinter6@gmail.com> * Fix oppia#18512: Graph Interaction Mobile Grey Out Bug (oppia#19532) * Fix Reset Button Styling on Graph * Change to Flow Root * Fix oppia#18079: Add fixed height to Skill dialogbox (oppia#19558) * added fixed height * fixed search bar * lint --------- Co-authored-by: Nikita Vorobev <vorobyev.nikita@gmail.com> * Fix oppia#19398 Added make stop in case of non zero exit code (oppia#19483) * Trap command used to check for Sudden Exit of running tests on code * trap command is added with sigint handler * trap command is added with sigint handler * trap command is added with sigint handler * trap command is added with sigint handler * trap command is added with sigint handler * changed done as reuqired * coe updated to check for exit aprt from keyboard interrupt also * code for all tests updated in makeflie to stop containers in any case of failure * code for all tests updated in makeflie to stop containers in any case of failure * make stop will executed for all exit code apart from 0 now * removed extra spaces * removed the extra echo commands * backend tests workflow changed * backend tests workflow changed * removed the extra echo commands * Fixes google-chrome not found for Mac users in Docker setup (oppia#19512) * fix: chrome version for mac * chore: removed newlines * chore: refracted the variables name * Fix oppia#19545: Exploration editor category creating new categories when selecting old ones. (oppia#19549) * Fix Exploration Editor Category Select * Change to Value Change * Fix oppia#19536: Adds hover effect to subscribe button on android page (oppia#19565) * button replaced by primary-button in android page, also disabled attribute added * fix linter error * Creates voiceover language policy model (oppia#19456) * Creates voiceover model * Adds test for entity voiceover model * Add voiceover domain and service methods. * Adds voiceover domain and services tests * Fixes mypy checks * Creates voiceover policy model * Fixes linter issues. * Address review comments * Adds regex check for language accent code * Updates voiceover policy model name. * Adds voiceover language accent constants * Adds language accent codes list for voiceovers * updates docstring * updates docstring * Deletes voice policy dir and shifts it into voiceover dir * Adds helper methods to get & save in VoiceoverAutogenerationPolicyModel * Creates json files to stores L-a constants. * Adds test to validate voiceover language accent constants. * Removes dead code * Updates code comment * Updates code comment * Updates Readme. * Adds note to developers * Updates code comment * GSoC_2.5: Frontend changes to enable pinning of opportunties (oppia#19122) * Front-end changes * Fix frontend tests * Fix frontend tests * Fix linting * Fix tests * Fix tests * Fix tests * Fix linting * Fix linting * Fix linting * Fix CI checks * Fix all linting errors * Update the frontend files and write unit tests * Fix failing unit tests * Fix failing unit tests * Fix failing unit tests * Fix failing unit tests * Fix failing CI checks * Fix failing unit tests * Fix CI Checks * Fix lint checks * Fix typescript typecheck errors * Fix typescript typecheck errors * Fix typescript typecheck errors * Address Review Comments * Fix flaky tests * Fix lints and add coverage tests * Fix lints and add coverage tests * Fix oppia#15377: Interaction changes still remain in pop-up after cancellation (oppia#19579) Fix Customization Args Saved Memento * Fix issue oppia#19576:Library Page Search Results Alignment Issue (oppia#19588) searched results alligned to center Co-authored-by: 2008sahil <getinter6@gmail.com> * use regex for matching * change regex * Fix oppia#19516: Navigation issues in the blog author page (oppia#19517) fix: Navigation issues in the blog author page * fix : [BUG]: Cursor should be hand pointer instead of text pointer oppia#19575 (oppia#19584) * Click-Arrow edit button text solved * some minor fixes * Fix issue oppia#19407:Blog page is not responsive on mobile devices (oppia#19474) * fixed UI changes in blog page * passing lint testcase1 * passing lint testcase2 * all lint case passed * all lint case passed * final commit * done suggested changes * Suggested Changes Done * added media query at 470px * fixed UI changes in blog page * passing lint testcase1 * passing lint testcase2 * all lint case passed * all lint case passed * final commit * done suggested changes * Suggested Changes Done * Resolved responsive issues in blog page * solved responsive issues * added margin to blog-navigate icon * fixed all changes * linter issue resolved * linter issue resolved * all linter checks resolved * changed naming to no-result-found --------- Co-authored-by: 2008sahil <getinter6@gmail.com> * FIX oppia#19571 : Add a space after "or" Conjunction and Second Option in Exploration Multi-selection. (oppia#19573) {{Fix spacing with OR}} Co-authored-by: Sean Lip <sean@seanlip.org> * Fix oppia#10647: Fixed faulty navbar on topic editor (oppia#19567) * fixed the navbar on subtopic page and also added necessary tests * removed unnecessary function * fixed coverage issues * added dynamic icon in navbar * fixed some linting issues * simplified the boolean expression * Fix oppia#19507: Corrected responsive design for exploration editor nav tools (oppia#19600) Fixed responsive issue with exploration editor * Remove unused dependencies: `py` (via `pytest` upgrade)and `PyGithub` (oppia#19553) * removed unused deps * removed unused tests * upgraded pytest to 7.4.4 * fix regex * fix docstring * fix tests * fix test * fix test --------- Co-authored-by: Nikita Vorobev <vorobyev.nikita@gmail.com> Co-authored-by: Yashwardhan Jyani <100014271+yashwardhan-jyani@users.noreply.github.com> Co-authored-by: Sahil Gupta <155609546+2008sahil@users.noreply.github.com> Co-authored-by: 2008sahil <getinter6@gmail.com> Co-authored-by: Justin Nguyen <70992422+jnvtnguyen@users.noreply.github.com> Co-authored-by: Harsh Keshari <108923011+hrshkshri@users.noreply.github.com> Co-authored-by: Yash Dugriyal <127002519+yash1378@users.noreply.github.com> Co-authored-by: Akash Jaiswal <akashjaiswal3846@gmail.com> Co-authored-by: sagar-subedi <66303548+sagar-subedi@users.noreply.github.com> Co-authored-by: Nikhil <nikhil.agarwal.2019@gmail.com> Co-authored-by: Ashwath Kannan <98253080+Ash-2k3@users.noreply.github.com> Co-authored-by: Afzal Khan <tricktomade@gmail.com> Co-authored-by: Yogesh Saini <97088265+Ykumar1415@users.noreply.github.com> Co-authored-by: Rahat <136263179+rahat2134@users.noreply.github.com> Co-authored-by: Sean Lip <sean@seanlip.org> Co-authored-by: Sambhav Kaushik <92575005+masterboy376@users.noreply.github.com> Co-authored-by: amyyeung17 <amyyeung17@gmail.com> Co-authored-by: Harshvardhan Singh <73544247+Lawful2002@users.noreply.github.com>
Overview
Start every language name in english first in language drop down.
Essential Checklist
Proof that changes are correct
Proof of changes on desktop with slow/throttled network
NA
Proof of changes on mobile phone
NA
Proof of changes in Arabic language
PR Pointers