Skip to content

styles(topMenu):space between .home and .hamburger#23624

Closed
aghArdeshir wants to merge 2 commits intoangular:masterfrom
aghArdeshir:patch-1
Closed

styles(topMenu):space between .home and .hamburger#23624
aghArdeshir wants to merge 2 commits intoangular:masterfrom
aghArdeshir:patch-1

Conversation

@aghArdeshir
Copy link
Contributor

@aghArdeshir aghArdeshir commented Apr 30, 2018

when hamburger menu is clicked, it's ripple background is drawn until the very edge of home's image, leaving no space. this adds a tiny space there

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[x ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

On the top header, when hamburger menu button is present, when clicked on it, a ripple background is drawn and is extended just to very edge of the main Angular image in home link.

Issue Number: N/A

What is the new behavior?

I added a tiny space, and in my opinion it just looks nicer

Does this PR introduce a breaking change?

[ ] Yes
[x ] No

Other information

this is just self-opinionated. I just saw it on website and felt like there needs to be a small space there

@gkalpak gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer comp: aio target: patch This PR is targeted for the next patch release labels Apr 30, 2018
@mary-poppins
Copy link

You can preview b73ca91 at https://pr23624-b73ca91.ngbuilds.io/.

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Thx for working on this. It has been bothering me for a while 😁
Adding some margin would also be more consistent with the Material Design guidelines.

I believe it would be more consistent to fix it by adjusting the hamburger's margin (since that is what we use for spacing the hamburger button and the logo.

So, can you please change the following lines (instead of adding a margin to the logo):

$hamburgerShownMargin: 0;
$hamburgerHiddenMargin: 0 24px 0 -88px;

$hamburgerShownMargin: 0 8px 0 0;
$hamburgerHiddenMargin: 0 16px 0 -88px;

Changing the "hidden" margin to 16px, makes it more consistent with the margin on the right side of the toolbar. And making the "shown" margin 8px looked better to me (because the hamburger button itself has plenty of padding). Open to different suggestions.

@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels May 1, 2018
@aghArdeshir
Copy link
Contributor Author

Thank you @gkalpak 😃
I changed exactly as you told me and also tried it out! Yep! That's the true solution! Thanks.

But there is still one tiny thing (very trivial) that bothers me 😆
and that is the outline when I click & hold on or drag logo.

screenshot-2018-5-2 screenshot from 2018-05-02 11-42-40 png png image 1366 x 768 pixels

as you see the the outline is started from the very beginning of image, but at the end there is 20px margin : ))))
But of course, that's so trivial.

Thank You! 👍

@mary-poppins
Copy link

You can preview a4ca306 at https://pr23624-a4ca306.ngbuilds.io/.

@gkalpak
Copy link
Member

gkalpak commented May 2, 2018

Awesome! Thx 👍

WRT the asymmetric outline:
I can't reproduce it, but let's fix it anyway 😃
We should move the right margin from the image (.nav-link.home img) to the anchor wrapper (.nav-link.home).

You can remove the margin-right style from here:

.nav-link.home img {
position: relative;
margin-top: -21px;
margin-right: 20px;

...and apply it to a.nav-link.home by changing:

a.nav-link {
margin: 0;
padding: 24px 16px;
cursor: pointer;
&:focus {
background: rgba($white, 0.15);
border-radius: 4px;
outline: none;
padding: 8px 16px;
}
}

...to:

 a.nav-link {
   margin: 0;
   padding: 24px 16px;
   cursor: pointer;
+
+  $.home {
+    margin-right: 20px;
+  }
  
   &:focus {
     background: rgba($white, 0.15);
     border-radius: 4px;
     outline: none;
     padding: 8px 16px;
   }
 }

BTW, it would be great if you could follow our commit message guidelines ❤️
(Read more about it here.)

@aghArdeshir
Copy link
Contributor Author

oops! sorry for those commit messages 😁
I hope you like this one :)

I did as you said! and its great right now! 😃 thanks!

Thank you so much for walking me through this. I'm learning so much.

+1

@mary-poppins
Copy link

You can preview e8c9d1a at https://pr23624-e8c9d1a.ngbuilds.io/.

Ardeshir81 added 2 commits May 2, 2018 16:54
When the `.hamburger` icon is clicked, it's background is drawn until
the very edge of `.home`'s image, leaving no space.
This makes the outline of `.home` symmetric.
@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker and removed state: WIP labels May 2, 2018
@gkalpak
Copy link
Member

gkalpak commented May 2, 2018

Thank you for working on this ❤️ This should be ready to get merged now 🎉
(BTW, I have changed the first two commit messages (and squashed the commits together) for you.)

@mary-poppins
Copy link

You can preview 6789f3b at https://pr23624-6789f3b.ngbuilds.io/.

IgorMinar pushed a commit that referenced this pull request May 2, 2018
When the `.hamburger` icon is clicked, it's background is drawn until
the very edge of `.home`'s image, leaving no space.

PR Close #23624
IgorMinar pushed a commit that referenced this pull request May 2, 2018
…#23624)

This makes the outline of `.home` symmetric.

PR Close #23624
@IgorMinar IgorMinar closed this in bb07fbd May 2, 2018
IgorMinar pushed a commit that referenced this pull request May 2, 2018
…#23624)

This makes the outline of `.home` symmetric.

PR Close #23624
@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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants