Skip to content

Set correct lang attribute#5479

Merged
bramkragten merged 5 commits intodevfrom
set-lang-attribute
Apr 8, 2020
Merged

Set correct lang attribute#5479
bramkragten merged 5 commits intodevfrom
set-lang-attribute

Conversation

@bramkragten
Copy link
Copy Markdown
Member

Breaking change

Proposed change

Fixes #5076

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Comment thread demo/src/html/index.html.template Outdated
@@ -1,5 +1,5 @@
<!DOCTYPE html>
<html lang="en">
<html lang="">
Copy link
Copy Markdown
Member

@balloob balloob Apr 7, 2020

Choose a reason for hiding this comment

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

Why not remove the attribute completely instead of initializing it as an empty value?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because an empty string means unknown language

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually, you are right, it should only be set to an empty string when you want to overrule a parent element to unknown.

Comment thread src/mixins/lit-localize-lite-mixin.ts Outdated
this._initializeLocalizeLite();
}
if (changedProperties.has("language")) {
document.querySelector("html")!.setAttribute("lang", this.language!);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't do this here. This mixin is added to a lot of elements, now they all update the lang tag

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As far as I know this is only used for elements that are not inside home-assistant like onboarding and auth with their own entrypoint?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

On authorize it would happen 3 times as there are 3 elements that use the mixin there, will look for a solution for that.

Comment thread src/mixins/lit-localize-lite-mixin.ts Outdated
Comment on lines +29 to +34
if (changedProperties.has("language")) {
const html = document.querySelector("html")!;
if (html.getAttribute("lang") !== this.language) {
html.setAttribute("lang", this.language!);
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Even if it's only used at a couple of pages, let's still not put it here. Let's put it in the main elements for onboarding/authorize.

@bramkragten bramkragten merged commit 4f70ec7 into dev Apr 8, 2020
@bramkragten bramkragten deleted the set-lang-attribute branch April 8, 2020 10:36
@bramkragten bramkragten mentioned this pull request Apr 14, 2020
@lock lock Bot locked and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

html lang="en" is hardcoded in templates

3 participants