feat(ui5-step-input): implement sap_horizon theme #4247

Todor-ads · 2021-11-03T05:45:56Z

feat(ui5-step-input): implement sap_horizon theme

ilhan007 · 2021-11-04T06:10:08Z

Check disabled state

openui5 - has border-bottom, more visible

wc - no border-bottom, almost invisible

-> check if you should add border or not and if the opacity/background color is fine

Readonly (but this is for all themes, not sap_horizon only)

wc - has -/+ icons, no not-editable icon (that has to be present in sap_horizon only)

openui5 - no -/+ icons, has not-editable icon

-> here, at least hide +/- icons when readonly (technically that means not to render the incr/decr divs and remove the padding around the internal ui5-input to make it stretch to the entire width of the ui5-step-input)
-> the not-editable icon will come from the ui5-input, so no action for you

ilhan007 · 2021-11-04T07:18:07Z

packages/main/src/StepInput.hbs

@@ -52,6 +54,7 @@
 	</ui5-input>

 	<!-- Increment Icon -->
+	{{#if hasNotReadOnly}}


you can use unless readonly, instead of if hasNotReadOnly

niyap · 2021-11-04T06:45:47Z

packages/main/src/themes/sap_horizon/StepInput-parameters.css

+:root {
+	--_ui5_step-input-input_error_background_color: inherit;
+	--_ui5-step_input_button_state_hover_backgroun_color: var(--sapField_Hover_Background);
+	--_ui5_step-input_border_style: none;
+	--_ui5_step-input_border_style_hover: none;
+	--_ui5_step-input_button_background-color: transparent;
+	--_ui5_step-input-input_border: none;
+	--_ui5_step-input-input_margin_top: 0;
+	--_ui5_step-input_button_display: inline-flex;
+	--_ui5_step-input_button_left: 0.125rem;
+	--_ui5_step-input_button_right: 0.0625rem;
+	--_ui5_step-input-input_border_focused_after: none;


Make all variables names consistent, using only underscore for example as in the second variable

niyap · 2021-11-04T06:50:21Z

packages/main/src/themes/StepInput.css

+:host([value-state=Success]:not([readonly]):not([disabled]):hover),
+:host([value-state=Error]:not([readonly]):not([disabled]):hover),
+:host([value-state=Information]:not([readonly]):not([disabled]):hover),
+:host([value-state=Warning]:not([readonly]):not([disabled]):hover) {
+	background-color: var(--_ui5-step_input_button_state_hover_backgroun_color);
+}
+
+:host([value-state=Success]:not([readonly]):not([disabled])) {
+	background-color: var(--sapField_SuccessBackground);
+}
+
+:host([value-state=Error]:not([readonly]):not([disabled])) {
+	background-color: var(--sapField_InvalidBackground);
+}
+
+:host([value-state=Information]:not([readonly]):not([disabled])) {
+	background-color: var(--sapField_InformationBackground);
+}
+
+:host([value-state=Warning]:not([readonly]):not([disabled])) {
+	background-color: var(--sapField_WarningBackground);
+}


When the component is active/focused, the background color should be the same as on hover. Currently, when the component is focused, it has white background only on hover, if you move away the pointer, the background is the same as in regular state, which is not correct

niyap · 2021-11-04T07:14:42Z

packages/main/src/themes/StepInput.css

 }

 :host .ui5-step-icon.ui5-step-inc {
-	right: 0;
+	right: var(--_ui5_step-input_button_right);
 }

 :host .ui5-step-icon *:not([clickable]),


In openUI5 when the increase/decrease button is active, there is a box-shadow and I think that it is the expected behaviour according to the requirement for the other inputs. Check the visualisation in openUI5 and make it the same here, when the button is active.

If you hover over the increase/decrease button, the component box-shadow in not visible. When you hover over the buttons, it should be the same as you hover over the input field.

niyap · 2021-11-04T07:22:04Z

packages/main/src/themes/StepInput.css

-  -webkit-appearance: none;
-  margin: 0;
+	-webkit-appearance: none;
+	margin: 0;
 }

 :host([disabled]) {


When the component is disabled, it should have opacity added over the regular state. Which means that we should have the same background image as in regular state and opacity on top

ilhan007 · 2021-11-04T20:38:21Z

Hello @SAP/ui5-webcomponents-topic-b should the UI5 Web Components core team take over and address the comments to complete the implementation of the StepInput. We would like to complete this at least tomorrow 5 Nov, Fri (although it should have been completed until 3, Wed)

niyap · 2021-11-05T09:28:31Z

packages/main/src/StepInput.js

@@ -186,6 +186,14 @@ const metadata = {
 			defaultValue: 0,
 		},

+		/**
+		 * Indicates whether the input is focssed


niyap · 2021-11-05T09:36:59Z

packages/main/src/StepInput.js

+			this.focused = true;
+			this.setAttribute("focused", "");


It is strange, on the first line you add focused attribute, on the second one you remove it?

niyap · 2021-11-05T09:37:12Z

packages/main/src/StepInput.js

+		focused: {
+			type: Boolean,
+		},
+


I could not understand why you add "focused" when you already have a property with the same function called "_focused"

niyap · 2021-11-05T09:49:12Z

packages/main/src/themes/StepInput.css

+:host([value-state=Error]:not([readonly]):not([disabled]):hover),
+:host([value-state=Information]:not([readonly]):not([disabled]):hover),
+:host([value-state=Warning]:not([readonly]):not([disabled]):hover) {
+	background-color: var(--_ui5_step_input_button_state_hover_backgroun_color);


Are you sure that the parameter resolves the correct value, since there is a typo

niyap · 2021-11-05T09:54:14Z

packages/main/src/themes/StepInput.css

 	height: 2rem;
 	height: 100%;
-	background-color: var(--sapField_Background);
+	background-color: var(--_ui5_step_input_button_background-color);


underscore for consistency

niyap · 2021-11-05T09:55:46Z

packages/main/src/themes/base/StepInput-parameters.css

+	--_ui5-step_input_button_state_hover_backgroun_color: var(--sapField_Background);
+	--_ui5_step_input_border_style: 1px solid var(--sapField_BorderColor);
+	--_ui5_step_input_border_style_hover: 1px solid var(--sapField_Hover_BorderColor);
+	--_ui5_step_input_button_background-color:var(--sapField_Background);


You have to change it here as well

niyap · 2021-11-05T09:58:23Z

packages/main/src/themes/sap_horizon/StepInput-parameters.css

+
+:root {
+	--_ui5_step_input_input_error_background_color: inherit;
+	--_ui5-step_input_button_state_hover_backgroun_color: var(--sapField_Hover_Background);


Here, the typos are two - dash between the ui5 and step and then "backgroun"

niyap · 2021-11-05T10:09:12Z

packages/main/src/themes/StepInput.css

+:host(:not([value-state]):not([readonly]):not([disabled])){
+	box-shadow: none;
 }


Even, with the fix in the input for the background image when disabled, it does not come out of the box below the buttons, you should adjust it

niyap

With the latest changes, I hope that everything works as expected. I have tested sap horizon, as well as fiori 3 and it looks fine.
I will approve the change, but as I am not part of the team, it will be good if somebody else from the team gives his approval that the change is reviewed from me.

tsanislavgatev · 2021-11-05T13:40:19Z

packages/main/src/themes/sap_horizon/StepInput-parameters.css

+	--_ui5_step_input_input_border_top_bottom_focused_after: 0;
+	--_ui5_step_input_input_border_radius_focused_after: 0.25rem;
+
+


Remove empty line

tsanislavgatev · 2021-11-05T13:42:18Z

packages/main/src/themes/StepInput.css

+	box-shadow: var(--_ui5-input-value-state-error-hover-box-shadow);
+}
+
+:host([value-state=Success]:not([readonly]):not([disabled]):hover),


Can you put the value state values in commas.

Example DatePicker.css
value-state="Success"

packages/main/src/themes/sap_horizon/StepInput-parameters.css

tsanislavgatev · 2021-11-05T14:51:54Z

I see that we now lost the Error state in HC themes:

Can we fix it before merging?

tsanislavgatev · 2021-11-05T15:06:05Z

If we exclude my last two comments, the change can be merged. And comments might be addressed in following issue. Depending on the priority, we can proceed and create followup issues.

ilhan007 · 2021-11-05T16:50:35Z

@Todor-ads
I suggest follow up on these visual issues (create a an issue or whatever you prefer), in Warning and Error states on high contrast themes
openui5:

webc

feat(ui5-step-input): implement sap_horizon theme

c9585be

Todor-ads requested a review from tsanislavgatev November 3, 2021 05:46

ilhan007 added the sap_horizon label Nov 3, 2021

Todor-ads removed the request for review from tsanislavgatev November 3, 2021 06:53

ilhan007 and others added 3 commits November 3, 2021 15:50

Merge branch 'master' into step_input_horizon

08d3cf3

fix focus

a714bd8

add correct colors

23927b6

Todor-ads requested review from tsanislavgatev, niyap and ilhan007 November 3, 2021 18:40

remove icons when stepInput has readonly

a4aa568

ilhan007 reviewed Nov 4, 2021

View reviewed changes

niyap requested changes Nov 4, 2021

View reviewed changes

Todor-ads added 2 commits November 5, 2021 09:54

fixed comments

c30fed3

fixed lint error

60c47fe

niyap reviewed Nov 5, 2021

View reviewed changes

ilhan007 requested review from niyap and ilhan007 November 5, 2021 10:21

Todor-ads added 4 commits November 5, 2021 12:39

fixed comments

d790bb9

fixed comments

a839991

fixed comments

a54b72e

fixed border color when hover in a fiori

a8e0a07

niyap previously approved these changes Nov 5, 2021

View reviewed changes

tsanislavgatev reviewed Nov 5, 2021

View reviewed changes

fixed comments

d144ec4

Todor-ads dismissed niyap’s stale review via d144ec4 November 5, 2021 14:07

tsanislavgatev reviewed Nov 5, 2021

View reviewed changes

packages/main/src/themes/sap_horizon/StepInput-parameters.css Show resolved Hide resolved

fixed comments

7edb1f7

tsanislavgatev approved these changes Nov 5, 2021

View reviewed changes

ilhan007 merged commit 4180fe7 into master Nov 5, 2021

ilhan007 deleted the step_input_horizon branch November 5, 2021 16:43

ndeshev pushed a commit to ndeshev/ui5-webcomponents that referenced this pull request Dec 13, 2021

feat(ui5-step-input): implement sap_horizon theme (SAP#4247)

16cbdcb

		--_ui5_step_input_input_border_top_bottom_focused_after: 0;
		--_ui5_step_input_input_border_radius_focused_after: 0.25rem;

feat(ui5-step-input): implement sap_horizon theme #4247

feat(ui5-step-input): implement sap_horizon theme #4247

Conversation

Todor-ads commented Nov 3, 2021 • edited Loading

ilhan007 commented Nov 4, 2021 • edited Loading

Check disabled state

Readonly (but this is for all themes, not sap_horizon only)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilhan007 commented Nov 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niyap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsanislavgatev commented Nov 5, 2021

tsanislavgatev commented Nov 5, 2021

ilhan007 commented Nov 5, 2021

Todor-ads commented Nov 3, 2021 •

edited

Loading

ilhan007 commented Nov 4, 2021 •

edited

Loading