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

Bump dependencies to stable and add RTL styles #37

Merged
merged 3 commits into from
May 6, 2020
Merged

Conversation

yuriy-fix
Copy link
Contributor

No description provided.

@yuriy-fix yuriy-fix marked this pull request as ready for review May 5, 2020 13:32
@yuriy-fix yuriy-fix changed the base branch from master to 1.1 May 5, 2020 13:33
@yuriy-fix
Copy link
Contributor Author

@diego, I have updated the screenshots, but I am not sure that it is the correct behavior in RTL with icon position switched. It looks like the value is clipped now

@DiegoCardoso
Copy link
Contributor

Yeah. That's coming from the vaadin-date-picker where we use direction: ltr on the [part=input-field]. It seems a bit off, I agree. In one hand it is consistent with the icon being opposite to the text. But that style also implies that the suffix and prefix for RTL are kept on the same position as in LTR.

Should we revert the flow-direction as well, so prefix would be on the right and suffix on the left of the input? (If we do so, maybe we should apply the same all email-field, number-field and decimal-field)

@Haprog
Copy link
Contributor

Haprog commented May 6, 2020

Looks like at least border radius is now missing from one of the elements in the middle section (between elements) in both LTR and RTL (looking at the updated screenshots).

@DiegoCardoso
Copy link
Contributor

Indeed. I had to zoom-in to catch that difference.

@DiegoCardoso
Copy link
Contributor

Found the issue with the borders. Because now vaadin-date-picker creates its own vaadin-date-picker-text-field instead of plain vaadin-text-field this line needs to be updated: https://github.com/vaadin/vaadin-date-time-picker/blob/master/src/vaadin-date-time-picker-date-picker.html#L37

@CLAassistant
Copy link

CLAassistant commented May 6, 2020

CLA assistant check
All committers have signed the CLA.

Comment on lines 7 to +12
<link rel="import" href="../../vaadin-text-field/src/vaadin-text-field.html">

<dom-module id="vaadin-date-time-picker-date-text-field-styless" theme-for="vaadin-date-time-picker-date-text-field">
<template>
<style>
:host([dir="rtl"]) [part="input-field"] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make more sense to import <link rel="import" href="../../vaadin-date-picker/src/vaadin-date-picker-text-field.html"> and then something like:

class DateTimePickerDateTextFieldElement extends customElements.get('vaadin-date-picker-text-field')

Then we wouldn't need to duplicate the RTL styles here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also noticed there's a typo in -styless"

Comment on lines +19 to +29

:not(*):placeholder-shown, /* to prevent broken styles on IE */
:host([dir="rtl"]) [part="value"]:placeholder-shown,
:host([dir="rtl"]) [part="input-field"] ::slotted(input:placeholder-shown) {
--_lumo-text-field-overflow-mask-image: none;
}

:host([dir="rtl"]) [part="value"],
:host([dir="rtl"]) [part="input-field"] ::slotted(input) {
--_lumo-text-field-overflow-mask-image: linear-gradient(to left, transparent, #000 1.25em);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess these can also be removed from here if this element is made to extend vaadin-date-picker-text-field?

Copy link
Contributor

@Haprog Haprog left a comment

Choose a reason for hiding this comment

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

Let's get rid of the RTL style duplication later. I'll create an issue for it. This is ok as is for now.

@DiegoCardoso DiegoCardoso changed the title Bump dependencies to stable Bump dependencies to stable and add RTL styles May 6, 2020
@DiegoCardoso DiegoCardoso merged commit 5553d6b into 1.1 May 6, 2020
@DiegoCardoso DiegoCardoso deleted the bump-deps branch May 6, 2020 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants