Feature/459 affix functionality for kirby input #2390

tspringborg · 2022-07-04T11:26:21Z

Why is this a draft?

I'd like to discuss some things I'm not completely happy with.

There are a couple of things.

In order to fix a compatibility issue when using the affix directive in conjunction with the date-mask directive, the affix directive simply removes the date-mask input field. Which doesn't seem to do anything atm. Is there a better way that doesn't require refactoring form field and related components? ..and is the extra date-mask input actually needed?
The CSS variables had to be added in quite a few places. Is there a better approach? (While still using CSS variables)

Which issue does this PR close?

This PR closes #459

What is the new behavior?

Added affix directive for kirby input fields

Does this PR introduce a breaking change?

Yes
No

Sort of. Since there's a compatibility issue with data-mask the affix directive will remove the extra input the date-mask adds.

Checklist:

The following tasks should be carried out in sequence in order to follow the process of contributing correctly.

Reminders

Make sure you have implemented tests following the guidelines in: "The good: Test".
Make sure you have updated the cookbook with examples and showcases (for bug fixes, enhancements & new components).

Review

Do a self-review.
Request that the changes are code-reviewed
Request that the changes are UX reviewed (only necessary if your PR introduces visual changes)

When the pull request has been approved it will be merged to develop by Team Kirby.

…kirby-input

tspringborg · 2022-07-05T09:04:26Z

apps/cookbook/src/app/examples/form-field-example/examples/input/default.ts

@@ -5,7 +5,7 @@ import { InputSize } from '@kirbydesign/designsystem';
 const config = {
  selector: 'cookbook-form-field-input-example',
  template: `<kirby-form-field>
-  <input kirby-input [size]="size" placeholder="Default input with placeholder text" />
+  <input kirby-input [size]="size" placeholder="Default input with placeholder text." />


a mistake. Disregard

jkaltoft

I've done a quick pre-review on the stylesheets (mostly about using Sass Modules).

Regarding...

The CSS variables had to be added in quite a few places. Is there a better approach? (While still using CSS variables)

...I'm convinced there may be a better approach, but I'll need some time to dig a bit deeper to come up with an idea for a solution.

jkaltoft · 2022-07-05T09:26:01Z

libs/core/src/scss/base/_variables.scss

+$form-field-input-font-family: var(--kirby-font-family);
+$form-field-input-line-height: 1.5;
+$form-field-input-padding: 1em;
+$form-field-label-height: 24px;


These Sass variables should stay in libs/designsystem/src/lib/components/form-field/_form-field-inputs.shared.scss

If they stay there. Then how can I import them without all the styles getting applied as well?

I'm not so familiar with Sass modules. So perhaps use will solve this?

See #2390 (comment)

When using form_field-inputs.shared, the importing scss file willl still apply all styles to the element importing.

So use does not solve it sadly...

jkaltoft · 2022-07-05T09:27:59Z

libs/designsystem/src/lib/components/form-field/_form-field-inputs.shared.scss

-$form-field-input-line-height: 1.5;
-$form-field-input-padding: 1em;
-$form-field-label-height: 24px;
+@import '~@kirbydesign/core/src/scss/base/variables';


We use Sass Modules, so @import should not be used.

See e.g. https://sass-lang.com/documentation/at-rules/use and https://css-tricks.com/introducing-sass-modules/

jkaltoft · 2022-07-05T09:29:03Z

libs/designsystem/src/lib/components/form-field/_form-field-inputs.shared.scss

-$form-field-input-line-height: 1.5;
-$form-field-input-padding: 1em;
-$form-field-label-height: 24px;
+@import '~@kirbydesign/core/src/scss/base/variables';


@kirbydesign/core/src/scss/base/variables is already included in @kirbydesign/core/src/scss/utils so this line should be removed

jkaltoft · 2022-07-05T09:33:16Z

libs/designsystem/src/lib/components/form-field/affix/input-affix.component.scss

@@ -0,0 +1,18 @@
+@import '~@kirbydesign/core/src/scss/base/variables';


Import other Sass files with @use instead of @import (Sass modules)

Suggested change

@import '~@kirbydesign/core/src/scss/base/variables';

@use '../form-field-inputs.shared' as shared;

(change the relative path if I got the wrong number of ..s 🙂)

jkaltoft · 2022-07-05T09:34:38Z

libs/designsystem/src/lib/components/form-field/affix/input-affix.component.scss

+@import '~@kirbydesign/core/src/scss/base/variables';
+
+:host {
+  --input-padding: #{$form-field-input-padding};


Suggested change

--input-padding: #{$form-field-input-padding};

--input-padding: #{shared.$form-field-input-padding};

jkaltoft · 2022-07-05T09:42:43Z

libs/designsystem/src/lib/components/form-field/affix/input-affix.component.scss

+  &.default {
+    font-size: 16px;
+    line-height: 24px;
+    letter-spacing: 0;
+    color: #707070;
+  }


Should use design tokens and utility functions - not hardcoded values. E.g. font-size: utils.font-size('n');. The same goes for colors.

stale · 2022-09-26T21:04:33Z

This pull request has been automatically marked as stale because of no recent activity. It will be closed in 10 weeks if no further activity occurs. Thank you for your contributions.

RasmusKjeldgaard · 2022-09-27T07:06:10Z

Replaced by #2406

thsp-jyskebank-dk added 8 commits June 23, 2022 12:47

preliminary setup of affixdirective. Examples, directive and module

36c852a

more work on affix feature

8c07779

Accounting for padding and a hack for date

7cbb598

comment on date hack

264575a

updated and cleaned up cookbook affix documentation

4fdf657

Merge branch 'develop' into 2368-enhancement-affix-functionality-for-…

0dcf50f

…kirby-input

basic affix test

1072068

spelling of affix directive basic test

396b216

jkaltoft requested a review from RasmusKjeldgaard July 4, 2022 11:37

thsp-jyskebank-dk added 2 commits July 4, 2022 14:11

added affix directive and component to 'public_api' lib

9a76bbf

correctly export AffixModule for lib

3925b30

github-actions bot temporarily deployed to pr-459-affix-functionality-for-kirby-input July 4, 2022 12:27 Inactive

tspringborg marked this pull request as ready for review July 5, 2022 08:59

tspringborg requested a review from jkaltoft as a code owner July 5, 2022 08:59

tspringborg commented Jul 5, 2022

View reviewed changes

jkaltoft requested changes Jul 5, 2022

View reviewed changes

tspringborg marked this pull request as draft July 5, 2022 10:13

kirbydesign deleted a comment from jkaltoft Jul 5, 2022

fix certain layout issues with affix directive

9fa52ad

github-actions bot temporarily deployed to pr-459-affix-functionality-for-kirby-input July 12, 2022 10:35 Inactive

thsp-jyskebank-dk added 2 commits July 12, 2022 16:11

set to OnPush to fix ngAfterViewChecked loop

1a39ecf

undo change to input example default

3c1cb9d

github-actions bot deployed to pr-459-affix-functionality-for-kirby-input July 18, 2022 08:06 View deployment

stale bot added the stale Automatically applied when there is no activity on an issue or PR label Sep 26, 2022

RasmusKjeldgaard closed this Sep 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/459 affix functionality for kirby input #2390

Feature/459 affix functionality for kirby input #2390

tspringborg commented Jul 4, 2022 •

edited

Loading

tspringborg Jul 5, 2022

jkaltoft left a comment

jkaltoft Jul 5, 2022

tspringborg Jul 5, 2022 •

edited

Loading

jkaltoft Jul 5, 2022

tspringborg Jul 12, 2022

jkaltoft Jul 5, 2022

jkaltoft Jul 5, 2022

jkaltoft Jul 5, 2022

jkaltoft Jul 5, 2022

jkaltoft Jul 5, 2022

stale bot commented Sep 26, 2022

RasmusKjeldgaard commented Sep 27, 2022

		@@ -0,0 +1,18 @@
		@import '~@kirbydesign/core/src/scss/base/variables';

	@import '~@kirbydesign/core/src/scss/base/variables';
	@use '../form-field-inputs.shared' as shared;

	--input-padding: #{$form-field-input-padding};
	--input-padding: #{shared.$form-field-input-padding};

Feature/459 affix functionality for kirby input #2390

Feature/459 affix functionality for kirby input #2390

Conversation

tspringborg commented Jul 4, 2022 • edited Loading

Why is this a draft?

Which issue does this PR close?

What is the new behavior?

Does this PR introduce a breaking change?

Checklist:

Reminders

Review

Choose a reason for hiding this comment

jkaltoft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tspringborg Jul 5, 2022 • edited Loading

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

stale bot commented Sep 26, 2022

RasmusKjeldgaard commented Sep 27, 2022

tspringborg commented Jul 4, 2022 •

edited

Loading

tspringborg Jul 5, 2022 •

edited

Loading