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

fix(cdk): refactor autofill directive #1775

Merged
merged 1 commit into from
May 23, 2022
Merged

Conversation

splincode
Copy link
Member

@splincode splincode commented May 17, 2022

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?

  • Bugfix
  • Feature
  • Refactoring
  • Code style update
  • Build or CI related changes
  • Documentation content changes

What is the current behavior?

Closes #1679 #1785

image

What is the new behavior?

Screen.Recording.2022-05-18.at.19.23.36.mov

image

Does this PR introduce a breaking change?

  • Yes
  • No

@lumberjack-bot
Copy link

lumberjack-bot bot commented May 17, 2022

Pull request was closed ✔️

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

@github-actions
Copy link
Contributor

github-actions bot commented May 17, 2022

Visit the preview URL for this PR (updated for commit a383428):

https://taiga-ui--pr1775-splincode-bugfix-iss-ydzrpwi9.web.app

(expires Sat, 21 May 2022 17:04:54 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@splincode splincode force-pushed the splincode/bugfix/issues/1679 branch from 2d8a555 to dc01b61 Compare May 18, 2022 16:24
@splincode splincode changed the title [WIP] fix(cdk): refactor autofill directive fix(cdk): refactor autofill directive May 18, 2022
@splincode splincode force-pushed the splincode/bugfix/issues/1679 branch 2 times, most recently from ac77796 to ebd8e70 Compare May 18, 2022 16:29
@tinkoff-bot tinkoff-bot force-pushed the splincode/bugfix/issues/1679 branch from ebd8e70 to c61de23 Compare May 18, 2022 16:33
@splincode splincode force-pushed the splincode/bugfix/issues/1679 branch from c61de23 to 4059975 Compare May 18, 2022 20:07
@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #1775 (a383428) into main (910ae9b) will increase coverage by 0.07%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             main    #1775      +/-   ##
==========================================
+ Coverage   63.17%   63.25%   +0.07%     
==========================================
  Files         930      931       +1     
  Lines       10050    10052       +2     
  Branches     1943     1945       +2     
==========================================
+ Hits         6349     6358       +9     
+ Misses       3251     3240      -11     
- Partials      450      454       +4     
Flag Coverage Δ
addon-charts 74.42% <ø> (ø)
addon-doc 26.73% <ø> (ø)
addon-editor 45.87% <ø> (ø)
addon-mobile ∅ <ø> (∅)
addon-table 79.59% <ø> (ø)
addon-tablebars ∅ <ø> (∅)
cdk 68.77% <77.77%> (+0.22%) ⬆️
core 68.00% <ø> (+0.18%) ⬆️
kit 63.16% <ø> (ø)
summary 63.25% <77.77%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
...cts/cdk/directives/autofilled/autofilled.module.ts 100.00% <ø> (ø)
.../cdk/directives/autofilled/autofilled.directive.ts 77.77% <75.00%> (+15.27%) ⬆️
...irectives/autofilled/autofilled-style.component.ts 100.00% <100.00%> (ø)
...imitive-textfield/primitive-textfield.component.ts 79.06% <0.00%> (+4.65%) ⬆️
projects/cdk/services/directive-styles.service.ts 92.85% <0.00%> (+7.14%) ⬆️
projects/cdk/tokens/default-renderer.ts 100.00% <0.00%> (+50.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 910ae9b...a383428. Read the comment docs.

@splincode splincode force-pushed the splincode/bugfix/issues/1679 branch 3 times, most recently from b554ab7 to 8557310 Compare May 19, 2022 07:43
@splincode splincode force-pushed the splincode/bugfix/issues/1679 branch 3 times, most recently from a34ceb2 to 8867bde Compare May 19, 2022 15:46
host: {
class: 'tui-autofill',
},
host: {class: 'tui-autofill'},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we no longer need this class, do we?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, transition: box-shadow 0.01s; we need to define rules only for those elements who input.tui-autofill or .tui-autofill input

})
export class TuiAutofilledDirective {
@HostBinding('class._autofilled')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this doesn't do anything, we add this class on host in PrimitiveTextfield and styles rely on it being there, not here.

Copy link
Member Author

Choose a reason for hiding this comment

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

<input (tuiAutofillChanges)="next.emit($event)">

But it would be great for us to add a class to the host to determine that it is autofilled.

_autofilled - system css class looks like _focused, etc

@splincode splincode force-pushed the splincode/bugfix/issues/1679 branch from 8e61328 to a383428 Compare May 20, 2022 17:00
@splincode
Copy link
Member Author

@nsbarsukov hello, please review me)

@splincode splincode merged commit 8656053 into main May 23, 2022
@splincode splincode deleted the splincode/bugfix/issues/1679 branch May 23, 2022 10:05
@well-done-bot
Copy link

well-done-bot bot commented May 23, 2022

'Well done'

@tinkoff-bot tinkoff-bot mentioned this pull request May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

🐞 - credit cards not showing up in safari
3 participants