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

RTL fix for range list #6417

Merged
merged 6 commits into from Jul 20, 2020
Merged

Conversation

yosilevy
Copy link
Contributor

Fixed direction in range list in date-range-picker

? html`<div slot="ranges" class="date-range-ranges">
? html`<div
slot="ranges"
class=${classMap({ "date-range-ranges": true, rtl: this._rtl })}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class=${classMap({ "date-range-ranges": true, rtl: this._rtl })}
class="date-range-ranges ${classMap({rtl: this._rtl })}"

Copy link
Member

Choose a reason for hiding this comment

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

Instead of setting a class, why not just set the direction (dir) attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Fixed. So priority is local attribute if applicable, classmap, custom CSS attribute.

protected updated(changedProps: PropertyValues) {
if (changedProps.has("hass")) {
const oldHass = changedProps.get("hass") as HomeAssistant | undefined;
if (!oldHass || oldHass.language !== this.hass.language) {
this._hour24format = this._compute24hourFormat();
}
if (
!oldHass ||
computeRTLDirection(oldHass) !== computeRTLDirection(this.hass)
Copy link
Member

Choose a reason for hiding this comment

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

Just compare the languages, you are now doing 2 computeRTLDirection extra.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But lang can change and RTLDirection may not change. It is inefficient however I though re-rendering was the thing to avoid at almost all cost??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I Joint both checks

Comment on lines 177 to 179
.date-range-ranges.rtl {
direction: rtl;
}
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 remove this now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought I did. Oops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -36,11 +37,14 @@ export class HaDateRangePicker extends LitElement {

@property({ type: Boolean }) private _hour24format = false;

@property({ type: String }) private _rtlDirection = "ltr";
Copy link
Member

Choose a reason for hiding this comment

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

let's not call this _rtlDirection as its value isn't a boolean, let's call it _textDirection or _direction or _dir

@yosilevy yosilevy merged commit d7e1e9e into home-assistant:dev Jul 20, 2020
@yosilevy yosilevy deleted the RTL-date-selector branch July 20, 2020 10:57
@bramkragten bramkragten mentioned this pull request Aug 3, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2022
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.

None yet

3 participants