-
Notifications
You must be signed in to change notification settings - Fork 891
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
[discover] optimize rendering language options #8685
[discover] optimize rendering language options #8685
Conversation
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8685 +/- ##
==========================================
- Coverage 60.86% 60.86% -0.01%
==========================================
Files 3793 3793
Lines 90447 90456 +9
Branches 14203 14204 +1
==========================================
+ Hits 55053 55057 +4
- Misses 31906 31910 +4
- Partials 3488 3489 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
const isAppSupported = | ||
!props.appName || language?.editorSupportedAppNames?.includes(props.appName); | ||
|
||
if (!isSupported || isBlocklisted || !isAppSupported) return; |
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 this be !isBlocklisted?
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.
tbh i added this advanced setting in case host wanted to prevent specific languages from showing but i doubt it will be used in the situation where this was in experimental and a language wasn't working. this should be removed in general.
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.
What is that advanced setting supposed to do?
|
||
const selectedLanguage = useMemo( | ||
() => ({ | ||
label: |
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.
does this need to default to something?
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.
like if it can't find the value? it could it might make sense to pull from the advanced setting
Signed-off-by: Kawika Avilla <[email protected]>
0173254
to
ee57410
Compare
Signed-off-by: Kawika Avilla <[email protected]>
❌ Create Or Update ContentSomething went wrong. Error creating or updating content in repository. Please try again. |
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
const isAppSupported = | ||
!props.appName || language?.editorSupportedAppNames?.includes(props.appName); | ||
|
||
if (!isSupported || isBlocklisted || !isAppSupported) return; |
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.
What is that advanced setting supposed to do?
|
||
languageService.setUserQueryLanguage(currentLanguage); |
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.
Was this what caused all those issues?
* memoization plus not setting the user query language Signed-off-by: Kawika Avilla <[email protected]> * clean up commented code Signed-off-by: Kawika Avilla <[email protected]> * Changeset file for PR #8685 created/updated * Changeset file for PR #8685 deleted --------- Signed-off-by: Kawika Avilla <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 85e0767) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* memoization plus not setting the user query language * clean up commented code * Changeset file for PR #8685 created/updated * Changeset file for PR #8685 deleted --------- (cherry picked from commit 85e0767) Signed-off-by: Kawika Avilla <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
* memoization plus not setting the user query language Signed-off-by: Kawika Avilla <[email protected]> * clean up commented code Signed-off-by: Kawika Avilla <[email protected]> * Changeset file for PR opensearch-project#8685 created/updated * Changeset file for PR opensearch-project#8685 deleted --------- Signed-off-by: Kawika Avilla <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
Optimize the language selector preventing too many renders and potentially setting of the language triggering an update.
Issues Resolved
n/a
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration