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 #805 #806

Merged
merged 2 commits into from
Oct 2, 2019
Merged

Fix #805 #806

merged 2 commits into from
Oct 2, 2019

Conversation

karakal
Copy link
Contributor

@karakal karakal commented Sep 27, 2019

#Fix of Error in addFilterColumn (see ticket #805 ).

@codecov
Copy link

codecov bot commented Sep 27, 2019

Codecov Report

Merging #806 into develop will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #806      +/-   ##
=============================================
+ Coverage      76.23%   76.28%   +0.04%     
- Complexity      2291     2293       +2     
=============================================
  Files            119      119              
  Lines           5404     5414      +10     
=============================================
+ Hits            4120     4130      +10     
  Misses          1284     1284
Impacted Files Coverage Δ Complexity Δ
src/FormField/Input.php 100% <0%> (ø) 32% <0%> (+2%) ⬆️
src/FormField/DropDown.php 92.68% <0%> (+0.09%) 39% <0%> (ø) ⬇️
src/FormField/Lookup.php 76.5% <0%> (+0.12%) 56% <0%> (ø) ⬇️
src/FormField/AutoComplete.php 66.31% <0%> (+0.35%) 33% <0%> (ø) ⬇️

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 dae88ae...5d61d81. Read the comment docs.

@acicovic acicovic self-requested a review October 1, 2019 18:22
to pass check
Copy link
Collaborator

@abbadon1334 abbadon1334 left a comment

Choose a reason for hiding this comment

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

@acicovic di you test it? i don't, but the issue is clear
LGTM ( i remove the extra line to pass the tests )
@karakal thanks!

@acicovic
Copy link
Collaborator

acicovic commented Oct 2, 2019

@abbadon1334 that was my thought also, but after testing I see that it doesn't behave exactly like TypeString as the column expands at maximum width. So the exception is indeed fixed but there should be an additional fix to make it work like string. I suspect the factoryType() function within FilterModel\Generic. Will take a look at it if I have time. @karakal if you have the time to spot and fix this, go ahead.

@acicovic
Copy link
Collaborator

acicovic commented Oct 2, 2019

OK, so this is by design. ATK assigns an atk-cell-expanded class in TableColumn\Text.php. This is what makes it look different, and for a text field this is logical. So merging this.

@acicovic acicovic merged commit 45c5130 into atk4:develop Oct 2, 2019
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.

3 participants