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(aio): fix search box overlap for small devices #17075

Merged
merged 1 commit into from
Jun 6, 2017

Conversation

sjtrimble
Copy link
Contributor

@sjtrimble sjtrimble commented May 28, 2017

Closes #17054
Closes #17007

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@sjtrimble sjtrimble added comp: aio action: review The PR is still awaiting reviews from at least one requested reviewer labels May 28, 2017
@mary-poppins
Copy link

The angular.io preview for 42db1da is available here.

@gkalpak gkalpak added this to REVIEW in docs-infra May 29, 2017
@@ -109,6 +109,7 @@ aio-search-box input {
padding: 5px 16px;
margin-left: 8px;
width: 150px;
min-width: 100px;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this changes anything 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just handles smaller screens better so that the Angular icon/log is not sitting directly next to the search bar. It really only applies to iPhone 5 and smaller.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how it affects anything. You have explicit width for smaller screens (180px), so setting min-width to 100px won't have any effect, right?

Copy link
Member

Choose a reason for hiding this comment

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

I think that the mixin below switches width to 50% when the big breakpoint is triggered?

Copy link
Member

@gkalpak gkalpak May 30, 2017

Choose a reason for hiding this comment

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

The big breakpoint is min-width: 1000px. The width is still 150px (which is larger than 100px, so min-width: 100px should have no effect) and on focus the width becomes 1000px * 50% = 500px (which again is larger than 100px) 😕

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. Back to you @sjtrimble

@sjtrimble sjtrimble force-pushed the search-box-overlap branch 2 times, most recently from 6dac8b4 to fa3d1ee Compare May 31, 2017 16:03
@sjtrimble
Copy link
Contributor Author

@gkalpak take a look and see if this makes more sense :)

width: 150px;
height: 40%;
width: 180px;
max-width: 240px;
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of max-width here, since you already specified width?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the animation doesn't pop out of it's 'box'.

Copy link
Member

Choose a reason for hiding this comment

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

I see. You can remove the max-width: 240px from the @include bp(big) section below.

@@ -86,7 +86,7 @@ md-toolbar.mat-toolbar {
}
}

aio-shell.sidenav-open {
aio-shell.page-home.sidenav-open {
Copy link
Member

Choose a reason for hiding this comment

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

Why only on home page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the sidenav content drops below the non-transparent toolbar on all other pages.

Copy link
Member

Choose a reason for hiding this comment

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

This has nothing to do with the overlap (and the styles below will not "fix" it). The actual issue on the home page is that the top bar is transparent.

@mary-poppins
Copy link

The angular.io preview for c54ed35 is available here.

- Adjust search box height
- Adjust search box standard width and width for smaller devices
- Fix search jump outside of specified max width
@mary-poppins
Copy link

The angular.io preview for 6e0d17b is available here.

@IgorMinar IgorMinar self-requested a review June 5, 2017 19:01
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

This looks behaves right on the preview server. I'll let @gkalpak take one more look before we merge this in, since his first comment hasn't been addressed yet.

@IgorMinar IgorMinar merged commit 76b4b80 into angular:master Jun 6, 2017
@petebacondarwin petebacondarwin removed this from REVIEW in docs-infra Jun 6, 2017
gkalpak added a commit to gkalpak/angular that referenced this pull request Jun 6, 2017
Restore the changes introduced in angular#17075, which wre accidentally overwritten
while rebasing angular#17155. Also, simplify the topbar positioning rules.
@gkalpak
Copy link
Member

gkalpak commented Jun 6, 2017

My coments have either been addressed or overwritten by subsequent PRs, so this LGTM 😁

gkalpak added a commit to gkalpak/angular that referenced this pull request Jun 6, 2017
Restore the changes introduced in angular#17075, which wre accidentally overwritten
while rebasing angular#17155. Also, simplify the topbar positioning rules.
alxhub pushed a commit that referenced this pull request Jun 6, 2017
Restore the changes introduced in #17075, which wre accidentally overwritten
while rebasing #17155. Also, simplify the topbar positioning rules.
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
- Adjust search box height
- Adjust search box standard width and width for smaller devices
- Fix search jump outside of specified max width
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
Restore the changes introduced in angular#17075, which wre accidentally overwritten
while rebasing angular#17155. Also, simplify the topbar positioning rules.
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
- Adjust search box height
- Adjust search box standard width and width for smaller devices
- Fix search jump outside of specified max width
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
Restore the changes introduced in angular#17075, which wre accidentally overwritten
while rebasing angular#17155. Also, simplify the topbar positioning rules.
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer cla: yes
Projects
None yet
6 participants