Skip to content

Rtl menu fix#12561

Merged
bramkragten merged 17 commits intohome-assistant:devfrom
yosilevy:RTL-menu-fix
May 11, 2022
Merged

Rtl menu fix#12561
bramkragten merged 17 commits intohome-assistant:devfrom
yosilevy:RTL-menu-fix

Conversation

@yosilevy
Copy link
Copy Markdown
Contributor

@yosilevy yosilevy commented May 3, 2022

Breaking change

Proposed change

mwc-menu and list item do not support RTL.
Further more the style fix needs to be applied to the span within the shadow dom of the list element. The list element is slotted under the menu so there is no way to get to the internal span via plain css in an elegant way.
In addition hovering over the menu causes re-renders due to the ripple which makes any style change to be reset.

The proposed code adds a new style when each menu item is created and then toggles the class on/off whenever an update to a menu item occurs.
It's not elegant but there's no other way that works.
Since MWC is not going to fix their very poor RTL support any time soon, I suggest we accept this change.

Before:
image

After:
image

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 or discussion:
  • 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:

protected firstUpdated(changedProps): void {
super.firstUpdated(changedProps);

this.updateComplete.then(() => {
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.

Can we do this only when in rtl mode?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done (getting hass with selector - equal to CSS host-context so no big deal).

@yosilevy
Copy link
Copy Markdown
Contributor Author

yosilevy commented May 5, 2022

@bramkragten While fixing clickable-list-item I found that the render bug was due to first-child. first-of-type does not have the bug so the whole override is not required anymore. Much cleaner solution now.

protected firstUpdated(changedProps): void {
super.firstUpdated(changedProps);

document.querySelector("home-assistant")!.provideHass(this);
Copy link
Copy Markdown
Member

@bramkragten bramkragten May 10, 2022

Choose a reason for hiding this comment

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

Can we use document.dir instead of querying hass?

We can add: document.dir = computeRTL(hass) ? "rtl" : "ltr"; in the translations-mixin

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


private _applyTranslations(hass: HomeAssistant) {
document.querySelector("html")!.setAttribute("lang", hass.language);
this.style.direction = computeRTL(hass) ? "rtl" : "ltr";
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 don't need/use this somewhere now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No. I tested it and it works fine when direction is on body and not on home-assistant tag. All my previous fixes will work because host-context would just climb 1 level up.

bramkragten
bramkragten previously approved these changes May 10, 2022
protected firstUpdated(changedProps): void {
super.firstUpdated(changedProps);

// @ts-ignore
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.

Why do we need to ignore this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Left over from previous code. Removing.

@bramkragten bramkragten merged commit 7a9c2f5 into home-assistant:dev May 11, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 12, 2022
@yosilevy yosilevy deleted the RTL-menu-fix branch May 19, 2022 17:52
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.

3 participants