Skip to content
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

LPD-39583 - Check if selectedItem exists before referencing it #4510

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kresimir-coko
Copy link
Collaborator

Jira ticket

TranslationManager Poshi tests were failing, I think I figured out why. The fix is in the commit, locally all tests pass. Time to see what CI has to say

image

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering the following test suites:

  •     ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 5 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 75cdfe67b3f03ae0eaaddbcb91187877e279f954

Sender Branch:

Branch Name: LPD-39583
Branch GIT ID: 7703ec1715e36c277d5904f2e4f9badd6adda94b

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@kresimir-coko
Copy link
Collaborator Author

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 24 out of 24 jobs passed

✔️ ci:test:relevant - 29 out of 29 jobs passed in 57 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 75cdfe67b3f03ae0eaaddbcb91187877e279f954

Upstream Comparison:

Branch GIT ID: 20c16b763e47400f6e4d7cc1f462a054278d1d4b
Jenkins Build URL: EE Development Acceptance (master) - 940 - 2024-10-22[08:38:15]

ci:test:stable - 24 out of 24 jobs PASSED
24 Successful Jobs:
ci:test:relevant - 29 out of 29 jobs PASSED
29 Successful Jobs:
For more details click here.
Test bundle downloads:

@liferay-continuous-integration
Copy link
Collaborator

@kresimir-coko
Copy link
Collaborator Author

Seems that the fix to the implementation was enough @dsanz

@juanjofgliferay
Copy link
Collaborator

@kresimir-coko Your fix is working, as it removes the errors that throws the component. However, I would say that there is still a hidden bug.
In the first example of the React component (frontend-js-components-sample-web) the button does not show the flag, but it has selected a default language.

image

It seems that the problem could be related with the selectedItem of the component when adminMode is off, see https://github.com/liferay-frontend/liferay-portal/pull/4510/files#diff-12217acb262642b4fc9915a451195df883d248a1e003fadd9909426c5f0f2fe1R220

Using the defaultLanguageId as fallback seems to fix the component errors and it displays the flag.

   selectedItem={activeLocales.find(
	(locale) => locale.id === selectedLanguageId || locale.id === defaultLanguageId
   )}

@kresimir-coko
Copy link
Collaborator Author

Good catch @juanjofgliferay I'll improve the solution 👍

Copy link
Collaborator

@juanjofgliferay juanjofgliferay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants