fix(kit): icon in date-related components occupies space when hidden #2966

theorlovsky · 2022-10-26T14:41:59Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows Conventional Commits
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Closes #2960

What is the new behavior?

Icon is completely hidden when removed via DI

Does this PR introduce a breaking change?

Yes
No

splincode · 2022-10-27T09:54:36Z

projects/kit/components/input-date-range/input-date-range.template.html

@@ -11,7 +11,7 @@
        automation-id="tui-input-date-range__textfield"
        tuiValueAccessor
        class="t-textfield"
-        [tuiTextfieldIcon]="computedMobile ? mobileIconContent : iconContent"
+        [tuiTextfieldIcon]="hasCalendarIcon ? (computedMobile ? mobileIconContent : iconContent) : null"


what do you think about implementing the pipe?

[tuiTextfieldIcon]="calendarIcon | tuiPresent ? (computedMobile ? mobileIconContent : iconContent) : null"

looks like a good idea

There no need for a pipe like that, it only complicates things, besides empty string is also lack of icon, not just null. There's also no need for nesting a ternary operator, you can write it like that:

[tuiTextfieldIcon]="computedMobile ? mobileIconContent : (calendarIcon && iconContent)"

If calendarIcon is falsy (empty string or null) it is passed to tuiTextfieldIcon, disabling the icon, otherwise iconContent is used.

yes, but what about mobileIconContent? it should not be present as well

besides, an empty string still renders tui-svg, it just has nothing to show. while null doesn't render anything

As for the pipe, it would be a useful addition to the cdk but I don't think we really need it here.

@waterplea do you want me to move the pipe to a separate PR?

as for this one

[tuiTextfieldIcon]="calendarIcon && iconContent"

I still don't think that this is a right solution. if calendarIcon is an empty string, then the expression '' && iconContent becomes just '' and you'll get an empty tui-svg rendered, as shown on the screenshot above. so the bug with an empty space next to a cleaner won't be fixed (which is a purpose of this PR)

and I've just figured out that because of this very reason we don't need the pipe here 🙂
because it filters out only nil values, but we need an empty string to be removed as well

how do you handle such cases in Taiga?

onIconClick(): void { if (!this.computedMobile) { // <-- this.isMobile && !!this.mobileCalendar return; } this.dialogService .open<TuiDay>(new PolymorpheusComponent(this.mobileCalendar, this.injector), { // 'Type<object> | null' is not assignable... ^^^^^^^^^^^^ }

I could add ! to mobileCalendar, but it's not type safe, and the linter is also not happy with this solution

just check it again?

if (!this.computedMobile || !this.mobileCalendar) { return; }

projects/kit/components/input-date-time/input-date-time.template.html

lumberjack-bot · 2022-10-28T09:32:43Z

Pull request was closed ✔️

All saved screenshots (for current PR) were deleted 🗑️

theorlovsky · 2022-10-28T09:49:50Z

I forgot to add docs for tuiIsPresent pipe, will do it now

projects/cdk/pipes/is-present/is-present.pipe.ts

bundlemon · 2022-10-28T09:55:45Z

BundleMon

Files updated (1)

Status	Path	Size	Limits
✅	demo/browser/main.(hash).js	323.08KB (-178B -0.05%)	+10%

Unchanged files (4)

Status	Path	Size	Limits
✅	demo/browser/vendor.(hash).js	205.38KB	+10%
✅	demo/browser/runtime.(hash).js	34.05KB	+10%
✅	demo/browser/polyfills.(hash).js	19.92KB	+10%
✅	demo/browser/scripts.(hash).js	14.92KB	+10%

Total files change -178B -0.03%

Groups updated (1)

Status	Path	Size	Limits
✅	demo/browser/*..js	2.17MB (-178B -0.01%)	-

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

codecov · 2022-10-28T10:05:33Z

Codecov Report

Base: 61.79% // Head: 61.89% // Increases project coverage by +0.10% 🎉

Coverage data is based on head (c6627c0) compared to base (3c219c1).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2966      +/-   ##
==========================================
+ Coverage   61.79%   61.89%   +0.10%     
==========================================
  Files        1473     1483      +10     
  Lines       17403    17559     +156     
  Branches     2364     2395      +31     
==========================================
+ Hits        10754    10868     +114     
- Misses       6210     6253      +43     
+ Partials      439      438       -1

Flag	Coverage Δ
addon-charts	`72.48% <ø> (ø)`
addon-doc	`48.46% <ø> (ø)`
addon-editor	`51.59% <ø> (-1.62%)`	⬇️
addon-mobile	`62.31% <ø> (+0.62%)`	⬆️
addon-table	`64.06% <ø> (ø)`
addon-tablebars	`98.07% <ø> (ø)`
cdk	`40.26% <ø> (-0.02%)`	⬇️
core	`76.79% <ø> (+0.56%)`	⬆️
kit	`74.14% <0.00%> (+0.51%)`	⬆️
summary	`61.89% <0.00%> (+0.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...nts/input-date-range/input-date-range.component.ts	`75.00% <0.00%> (+0.68%)`	⬆️
.../kit/components/input-date/input-date.component.ts	`85.52% <0.00%> (+1.11%)`	⬆️
...onents/color-selector/palette/palette.component.ts	`85.71% <0.00%> (-14.29%)`	⬇️
...omponents/editor-socket/editor-socket.component.ts	`28.57% <0.00%> (-4.77%)`	⬇️
...addon-editor/components/editor/editor.component.ts	`28.26% <0.00%> (-1.29%)`	⬇️
...don-editor/components/toolbar/toolbar.component.ts	`12.94% <0.00%> (-1.17%)`	⬇️
...nents/mobile-calendar/mobile-calendar.component.ts	`46.03% <0.00%> (-1.08%)`	⬇️
.../directives/tiptap-editor/tiptap-editor.service.ts	`9.18% <0.00%> (-0.20%)`	⬇️
projects/addon-editor/tokens/i18n.ts	`100.00% <0.00%> (ø)`
projects/kit/components/push/index.ts	`100.00% <0.00%> (ø)`
... and 38 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

projects/cdk/pipes/is-present/is-present.pipe.ts

projects/demo/src/modules/app/pages.ts

theorlovsky · 2022-11-01T12:40:57Z

moved tuiIsPresent pipe to a separate PR #3002

splincode · 2022-11-01T13:00:57Z

projects/kit/components/input-month-range/input-month-range.template.html

@@ -12,7 +12,7 @@
        [disabled]="computedDisabled"
        [nativeId]="nativeId"
        [readOnly]="readOnly"
-        [tuiTextfieldIcon]="calendarIcon"
+        [tuiTextfieldIcon]="calendarIcon ? calendarIcon : null"


what about :))

[tuiTextfieldIcon]="calendarIcon | tuiFallback: null"

where

export class TuiFallbackPipe { transform<T, K = T>(value?: T|null, fallback: K): T | K { return tuiIsPresent(value) ? value : fallback; } }

I don't mind, but I want to here @waterplea's opinion on this one, since it's our favourite topic in this PR 😄

but again, it doesn't cover a case with an empty string

Oh, sorry, you can use simple solution

[tuiTextfieldIcon]="calendarIcon ?? null"

instead of

[tuiTextfieldIcon]="calendarIcon ? calendarIcon : null"

oh right, I didn't even think about it for some reason

splincode · 2022-11-01T13:01:47Z

projects/kit/components/input-date/input-date.component.ts

@@ -225,10 +225,8 @@ export class TuiInputDateComponent
        return this.activeItem ? `` : filler;
    }

-    onMobileClick(): void {


breaking changes?

a method name changed, yes, but other than that it should work as before, I didn't notice any difference

theorlovsky · 2022-11-02T09:44:47Z

@splincode @waterplea so, everything got a lot simpler. I hope no one will come with an idea of hiding the icon using an empty string, and if they do, I wouldn't consider the result a bug on the Taiga side. so we're free to write as you suggested:

[tuiTextfieldIcon]="calendarIcon && iconContent"

as for this one

[tuiTextfieldIcon]="calendarIcon ? calendarIcon : null"

it can easily be reverted to the original

[tuiTextfieldIcon]="calendarIcon"

theorlovsky · 2022-11-03T11:40:46Z

@splincode since Alex is unavailable now, can we get someone else involved to get two approvals?

theorlovsky · 2022-11-03T13:06:59Z

[tuiTextfieldIcon]="calendarIcon"

@splincode what do you mean?

splincode · 2022-11-03T19:14:27Z

@theorlovsky sorry, I mean use [tuiTextfieldIcon]="calendarIcon && iconContent"

theorlovsky · 2022-11-03T19:21:15Z

@theorlovsky sorry, I mean use [tuiTextfieldIcon]="calendarIcon && iconContent"

@splincode I already use it

splincode · 2022-11-03T19:27:17Z

@theorlovsky sorry, it's good now

theorlovsky · 2022-11-04T11:04:51Z

@splincode how can we get a second approval here?

splincode · 2022-11-04T11:25:58Z

@vladimirpotekhin @nsbarsukov fyi

well-done-bot · 2022-11-09T13:36:34Z

theorlovsky requested review from waterplea, MarsiBarsi, vladimirpotekhin and nsbarsukov as code owners October 26, 2022 14:42

splincode reviewed Oct 27, 2022

View reviewed changes

projects/kit/components/input-date-time/input-date-time.template.html Outdated Show resolved Hide resolved

theorlovsky force-pushed the 2960 branch from 6b577b5 to 668bbf9 Compare October 28, 2022 09:32

theorlovsky requested review from splincode and waterplea and removed request for MarsiBarsi, waterplea, nsbarsukov, vladimirpotekhin and splincode October 28, 2022 09:43

splincode approved these changes Oct 28, 2022

View reviewed changes

splincode reviewed Oct 28, 2022

View reviewed changes

projects/cdk/pipes/is-present/is-present.pipe.ts Outdated Show resolved Hide resolved

theorlovsky force-pushed the 2960 branch from 668bbf9 to b687fa8 Compare October 28, 2022 11:02

theorlovsky requested review from splincode and removed request for waterplea October 28, 2022 11:03

splincode requested a review from waterplea October 28, 2022 11:36

waterplea reviewed Oct 31, 2022

View reviewed changes

projects/cdk/pipes/is-present/is-present.pipe.ts Outdated Show resolved Hide resolved

waterplea reviewed Oct 31, 2022

View reviewed changes

projects/demo/src/modules/app/pages.ts Outdated Show resolved Hide resolved

theorlovsky requested review from waterplea and removed request for splincode October 31, 2022 13:52

theorlovsky force-pushed the 2960 branch from b687fa8 to 848e7d2 Compare November 1, 2022 12:39

splincode reviewed Nov 1, 2022

View reviewed changes

splincode approved these changes Nov 1, 2022

View reviewed changes

splincode reviewed Nov 1, 2022

View reviewed changes

fix(kit): icon in date-related components occupies space when hidden

c6627c0

theorlovsky force-pushed the 2960 branch from 848e7d2 to c6627c0 Compare November 2, 2022 09:35

waterplea approved these changes Nov 9, 2022

View reviewed changes

waterplea merged commit 27c2d79 into taiga-family:main Nov 9, 2022

theorlovsky deleted the 2960 branch November 9, 2022 19:51

tinkoff-bot mentioned this pull request Nov 16, 2022

🚀 Release/v3.10.0 #3073

Merged

splincode mentioned this pull request Jan 18, 2023

🐞 - error TS2339: Property 'onMobileClick' does not exist on type 'TuiInputDateComponent' since 3.10.0 #3448

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(kit): icon in date-related components occupies space when hidden #2966

fix(kit): icon in date-related components occupies space when hidden #2966

theorlovsky commented Oct 26, 2022 •

edited

Loading

splincode Oct 27, 2022

theorlovsky Oct 27, 2022

waterplea Oct 27, 2022

theorlovsky Oct 28, 2022

theorlovsky Oct 28, 2022 •

edited

Loading

waterplea Oct 28, 2022

theorlovsky Oct 31, 2022

theorlovsky Oct 31, 2022

theorlovsky Oct 31, 2022

theorlovsky Nov 1, 2022

lumberjack-bot bot commented Oct 28, 2022 •

edited

Loading

theorlovsky commented Oct 28, 2022

bundlemon bot commented Oct 28, 2022 •

edited

Loading

codecov bot commented Oct 28, 2022 •

edited

Loading

theorlovsky commented Nov 1, 2022

splincode Nov 1, 2022

theorlovsky Nov 1, 2022

theorlovsky Nov 1, 2022

splincode Nov 1, 2022 •

edited

Loading

theorlovsky Nov 2, 2022

splincode Nov 1, 2022

theorlovsky Nov 1, 2022

theorlovsky commented Nov 2, 2022

theorlovsky commented Nov 3, 2022

theorlovsky commented Nov 3, 2022

splincode commented Nov 3, 2022

theorlovsky commented Nov 3, 2022

splincode commented Nov 3, 2022

theorlovsky commented Nov 4, 2022

splincode commented Nov 4, 2022

well-done-bot bot commented Nov 9, 2022

fix(kit): icon in date-related components occupies space when hidden #2966

fix(kit): icon in date-related components occupies space when hidden #2966

Conversation

theorlovsky commented Oct 26, 2022 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

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

theorlovsky Oct 28, 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

lumberjack-bot bot commented Oct 28, 2022 • edited Loading

Pull request was closed ✔️

theorlovsky commented Oct 28, 2022

bundlemon bot commented Oct 28, 2022 • edited Loading

BundleMon

codecov bot commented Oct 28, 2022 • edited Loading

Codecov Report

theorlovsky commented Nov 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splincode Nov 1, 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

theorlovsky commented Nov 2, 2022

theorlovsky commented Nov 3, 2022

theorlovsky commented Nov 3, 2022

splincode commented Nov 3, 2022

theorlovsky commented Nov 3, 2022

splincode commented Nov 3, 2022

theorlovsky commented Nov 4, 2022

splincode commented Nov 4, 2022

well-done-bot bot commented Nov 9, 2022

theorlovsky commented Oct 26, 2022 •

edited

Loading

theorlovsky Oct 28, 2022 •

edited

Loading

lumberjack-bot bot commented Oct 28, 2022 •

edited

Loading

bundlemon bot commented Oct 28, 2022 •

edited

Loading

codecov bot commented Oct 28, 2022 •

edited

Loading

splincode Nov 1, 2022 •

edited

Loading