Skip to content
This repository was archived by the owner on Oct 2, 2019. It is now read-only.

fix(uiSelectCtrl) calculateContainerWidth use width of ui-select-container #1873

Merged
merged 2 commits into from
Mar 20, 2017
Merged

fix(uiSelectCtrl) calculateContainerWidth use width of ui-select-container #1873

merged 2 commits into from
Mar 20, 2017

Conversation

@bmsdave
Copy link
Contributor

@bmsdave bmsdave commented Dec 16, 2016

( not from body, if I set attribute append-to-body=true)

@bmsdave bmsdave mentioned this pull request Dec 16, 2016
Closed
@bmsdave
Copy link
Contributor Author

bmsdave commented Dec 16, 2016

These four failing tests (which are not related to this PR) seem to happen on a fresh checkout from master now. Not sure why, but I think it's probably a dependency-version thing. I will be investigating further.

@pete-otaqui
Copy link
Contributor

Related to #1872, suggest this PR should also account for actual padding of the field rather than a hard-coded 10px.

@pete-otaqui
Copy link
Contributor

Also the failing tests are logged as #1877

@bmsdave
Copy link
Contributor Author

bmsdave commented Jan 16, 2017

@pete-otaqui, Hi.
I need some of the actions that have been adopted this PR?

@bmsdave
Copy link
Contributor Author

bmsdave commented Feb 14, 2017

@Jefiozie, @pete-otaqui, hi. This PR is actualy. Pls, tell me what I should do to the issue #1874 was solved?

@Jefiozie
Copy link
Contributor

Jefiozie commented Feb 14, 2017 •
edited
Loading

I think you have added to much things
. Please clean up the select.js etc. Hope i can have a look later this week .

@bmsdave
Copy link
Contributor Author

bmsdave commented Feb 15, 2017

@JeffGreat, done.

@Jefiozie
Copy link
Contributor

Jefiozie commented Mar 7, 2017

@bmsdave could you rebase this? Thanks

@bmsdave
Copy link
Contributor Author

bmsdave commented Mar 7, 2017

@Jefiozie, done.

@Jefiozie
Copy link
Contributor

Jefiozie commented Mar 7, 2017

Could you also make a test around this? So that we now when behavior is changing?

Thanks

@bmsdave
Copy link
Contributor Author

bmsdave commented Mar 7, 2017 •
edited
Loading

@Jefiozie ,
OK. I'll do everything on the weekend! =)

@Jefiozie
Copy link
Contributor

Jefiozie commented Mar 7, 2017

Could you also have a look at: #1906 looks similar issue.

Thanks

@easyest
Copy link

easyest commented Mar 8, 2017

This line is still not changed:
var inputWidth = containerWidth - input.offsetLeft - 10;

At least in bootstrap there is no need to subtract 10 px because input width should be the same as button width.

@bmsdave
fix(uiSelectController): calculateContainerWidth use width of ui-sel…
7ad4ef1 
…ect-container

     not from body, if I set attribute `append-to-body=true`

    fix(uiSelectController): sizeSearchInput use container width - offsetLeft if updateInVisible

    fix(uiSelectController): add unit test
@bmsdave
Copy link
Contributor Author

bmsdave commented Mar 13, 2017

@Jefiozie, hi.
test and remove -10px is done.
plz, check it.

@Jefiozie Jefiozie self-requested a review March 13, 2017 11:21
@Jefiozie Jefiozie added this to the v0.19.6 milestone Mar 13, 2017
Jefiozie
Jefiozie approved these changes Mar 13, 2017
View reviewed changes
Copy link
Contributor

@Jefiozie Jefiozie left a comment

Choose a reason for hiding this comment

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

Looks good

@Jefiozie
Copy link
Contributor

Hi could you rebase this? After this I will merge the PR. Thanks

@bmsdave
Copy link
Contributor Author

bmsdave commented Mar 20, 2017

@Jefiozie, like that? i just resolve conflict.

@Jefiozie Jefiozie merged commit 08a0752 into angular-ui:master Mar 20, 2017
@lameyer lameyer mentioned this pull request Jun 28, 2017
Merged
lameyer added a commit to looker/ui-select that referenced this pull request Jun 29, 2017
@lameyer
Merge pull request #8 from looker/lameyer/update-to-0.19.7
858a94b 
Update to 0.19.8, with the changes in angular-ui#1873 reverted
@dmytro-krekota dmytro-krekota mentioned this pull request Mar 27, 2018
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@Jefiozie Jefiozie Jefiozie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bmsdave bmsdave

Labels

None yet

Projects

None yet

Milestone

v0.19.6

Development

Successfully merging this pull request may close these issues.

5 participants

@bmsdave @pete-otaqui @Jefiozie @easyest @NaturalL