-
Notifications
You must be signed in to change notification settings - Fork 239
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
Update applauncher masthead #1049
Update applauncher masthead #1049
Conversation
@mcarrano @matthewcarleton @jeff-phillips-18 @jgiardino would someone mind reviewing this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @michael-coker except that it looks like on the tall masthead versions the User menu (i.e Brain Johnson) is lower than the two icon menus to the left. Can you check that? Might be just my eyes but they don't look vertically aligned.
@mcarrano it kinda looked that way to me, too. Here are a couple of screenshots with guides that show the top and bottom of the middle (help) icon. Looks like the help and user icons are aligned to the bottom of the guide in each browser, but even though they're the same font size, they're different heights (by 1px). Want me to make an adjustment? firefoxchrome |
Yes, I see what's going on @michael-coker . The other thing that adds to this illusion is that the text "Brian Johnson" is bottom aligned with the icons rather than center-aligned. Before making more changes, I'd like @kybaker to take a look. Do you feel like the user icon and/or the menu should be adjusted to make these items appear in a straight line? Or do you think it's OK as is? |
571198b
to
fe884ef
Compare
@mcarrano I re-worked this so the text is center aligned, but the build on Travis is failing. I believe this is the error:
I pulled down master and rebased before updating the branch, but didn't change anything with the modal. @dgutride @mindreeper2420 do you know anything about that test? You can see the build output here - https://travis-ci.org/michael-coker/patternfly/builds/378927992 |
@mcarrano the vertical alignment updates are in the latest build if you want to review - https://rawgit.com/michael-coker/patternfly/update-applauncher-masthead-dist/dist/tests/application-launcher.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @michael-coker . Thanks for addressing the alignment.
src/less/application-launcher.less
Outdated
@@ -105,9 +105,8 @@ | |||
|
|||
.navbar-utility .applauncher-pf { | |||
.dropdown-menu { | |||
border-width: @applauncher-pf-menu-link-border-width !important; | |||
//border-width: @applauncher-pf-menu-link-border-width !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Removed.
@jeff-phillips-18 @dgutride can you merge this PR? |
Some RCUE sass files were not generated due to the timing of the rcue addition and modifications made in patternfly#1049
Description
Update masthead in app launcher examples to reflect new masthead design http://www.patternfly.org/pattern-library/application-framework/masthead/#design
fixes patternfly/patternfly#1031
Changes
Write a list of changes the PR introduces
Link to rawgit and/or image
https://rawgit.com/michael-coker/patternfly/update-applauncher-masthead-dist/dist/tests/application-launcher.html
PR checklist (if relevant)