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

[5.3] Minimize language selector in installation process #44735

Draft
wants to merge 4 commits into
base: 5.3-dev
Choose a base branch
from

Conversation

bembelimen
Copy link
Contributor

@bembelimen bembelimen commented Jan 14, 2025

This is an idea I had, and just want to see some opinions/improvements about it.

Summary of Changes

When installing a fresh Joomla! the first thing is the language selector. But most of the time, you don't need to change the language as it's auto discovered. So this PR minimize the box and moved the selector to a dialog popup. It also fixes some autodiscoveries when e.g. only "de/fr/nl" is submitted by the browser (Firefox does this) to discover "de-DE/fr-FR/nl-NL".

Testing Instructions

  • Do a fresh installation with the code of this PR

Actual result BEFORE applying this Pull Request

Language selector on top

image

Expected result AFTER applying this Pull Request

Language selector minimized

image

image

image

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.3-dev labels Jan 14, 2025
@softforge
Copy link
Contributor

I like this as it's cutting down the choices needed to set up a site and anything to make the install easier for the majority is good

@chmst
Copy link
Contributor

chmst commented Jan 15, 2025

I like the idea.
For A11y: The link must have an underline.

@fgsw
Copy link

fgsw commented Jan 15, 2025

related Issues: #44667, #24481.

$link = HTMLHelper::_('link', '#languageSelect', '', ['id' => 'languageForm-current', 'data-joomla-dialog' => '###dialogattr###', 'class' => 'btn btn-link ps-1']);

// Dialog needs single quotes for attributes, so use single quotes...
$link = str_replace('"###dialogattr###', '\'{"textHeader": "' . Text::_('INSTL_SELECT_INSTALL_LANG') . '", "iconHeader":"icon-language"}\'', $link);
Copy link
Member

Choose a reason for hiding this comment

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

Please use propper json_encode and html value encoding.
Because when translation will contain unexpected symbols, then whole code will be broken.

htmlspecialchars(json_encode($data));

Also can do without str_replace:

$link = HTMLHelper::_('link', 
  '#languageSelect', '', 
  [
    'id' => 'languageForm-current', 
    'data-joomla-dialog' => htmlspecialchars(json_encode($data)), 
    'class' => 'btn btn-link ps-1']);

@brianteeman
Copy link
Contributor

It is not obvious that clicking on the current language name will let you change the language. This is an accessibility failure.

@chmst
Copy link
Contributor

chmst commented Jan 15, 2025

The label could be "Change the currnet language "

margin: 0 .5rem;
}
.buttons-holder {
margin-left: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
margin-left: auto;
margin-inline-start: auto;

This ensures that code works as intended in RTL as well AS LTR.

RTL using margin-left
image

RTL using margin-inline-start
image

echo Text::sprintf('INSTL_SELECTED_INSTALL_LANGUAGE', $link);
?>
</div>
<template id="languageSelect">
Copy link
Member

Choose a reason for hiding this comment

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

You can keep it as regular element (just make it hidden), if you want.
It going to be something like this:

<div hidden>
  <div id="languageSelect">
    <div class="mb-3"><?php echo $this->form->renderField('language'); ?></div>
  </div>
</div>

And set dialog option preferredParent to body for it:

'data-joomla-dialog' => htmlspecialchars(json_encode([
  'textHeader' => Text::_('INSTL_SELECT_INSTALL_LANG'),
  'iconHeader' => 'icon-language',
  'preferredParent' => 'body',
])), 

@@ -111,6 +111,11 @@ public static function detectLanguage()
$browserLang = substr($browserLang, 0, strcspn($browserLang, ';'));
$primary_browserLang = substr($browserLang, 0, 2);

// Some browser return only "fr" or "de", so let's try to use it like "fr-fr" or "de-de"
Copy link
Member

Choose a reason for hiding this comment

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

E.g. for English this would not work as there is no en-en.

Copy link
Contributor

Choose a reason for hiding this comment

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

i did some tests and the only time I could get firefox to return http_accept_header with ONLY the language part and not the region is when the complete version is xx-XX

so de-DE returns as de
but de-AT returns as de-AT

as there is no combination en-EN then firefox would always return the full language eg en-US

in short (and assuming my test is accurate) this code is correct and @richard67 concern while valid will never happen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester PR-5.3-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants