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

Fix setting reading the search language cookie #1693

Closed
wants to merge 5 commits into from

Conversation

joelit
Copy link
Contributor

@joelit joelit commented Oct 9, 2024

Reasons for creating this PR

There was a bug reading the content language cookie, due to a method not being updated to follow the new cookie convention. This PR is built upon #1689 .

Link to relevant issue(s), if any

Description of the changes in this PR

Changed the name of the cookie

Known problems or uncertainties in this PR

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

Takala Joeli A and others added 5 commits October 8, 2024 13:09
Translations can be maintained with the Symfony-Lokalise combination (pull, push, extract)  but not yet by extracting from Vue code (#1648)

At first, there was some uncertainty about whether the system was actually updating the data correctly (deletion in Lokalise -> deletion locally as well), but it seems that it worked correctly after all. Although the feature branch can be merged, further testing should still be done just to be sure.

Add cypress tests for alphabetical index + add new test vocab

Add cypress test for alt labels

Add cypress files for alphabetical index and hierarchy

Move alphabetical index cypress tests from vocab-home.cy.js + add checks for spinners

Change test vocab name

Fix uriSpace

Fix uri space in test vocab
…ant cypress test to avoid a false positive test result
@joelit joelit added the bug label Oct 9, 2024
@joelit joelit added this to the 3.0 milestone Oct 9, 2024
@joelit joelit requested a review from osma October 9, 2024 06:50
@joelit joelit self-assigned this Oct 9, 2024
Copy link

sonarcloud bot commented Oct 9, 2024

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

This is not the right way to fix #1692. There are, or should be, two cookies: SKOSMOS_LANGUAGE to remember the UI language and SKOSMOS_SEARCH_LANG to remember the content/search language. Whether we actually need the latter is debatable (as explained in #1692) but mixing up these two doesn't help.

@joelit
Copy link
Contributor Author

joelit commented Oct 9, 2024

I don't think nothing sets the SKOSMOS_LANGUAGE cookie anymore, though. It looks like it's becoming dead code in 3.0.

@osma
Copy link
Member

osma commented Oct 9, 2024

I don't think nothing sets the SKOSMOS_LANGUAGE cookie anymore, though. It looks like it's becoming dead code in 3.0.

You're right that it's not being set by the current Skosmos 3 code. In the Skosmos 2 code, we set event handlers for the UI language selection links:

// Event handlers for the language selection links for setting the cookie
$('#language a').each( function(index, el) {
$(el).on('click', function() {
var langCode = el.id.substr(el.id.indexOf("-") + 1);
setLangCookie(langCode);
});
});

I think the same should be done for Skosmos 3. I can open a new issue about this.

@osma
Copy link
Member

osma commented Oct 9, 2024

Opened a new issue #1696

@joelit
Copy link
Contributor Author

joelit commented Dec 12, 2024

Closing this PR, as the preferred solution was to get rid of the search language cookie (PR #1720). For example, the content language state of one vocabulary might not make sense when switching to another vocabulary with different supported languages. Better to handle search language with clang url parameter.

@joelit joelit closed this Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR)
Development

Successfully merging this pull request may close these issues.

Content language setting is not working consistently in Skosmos 3
2 participants