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

Masthead #50

Merged
merged 8 commits into from
Jul 6, 2017
Merged

Masthead #50

merged 8 commits into from
Jul 6, 2017

Conversation

jgiardino
Copy link

@jgiardino jgiardino commented Mar 23, 2017

https://jgiardino.github.io/patternfly-atomic/?p=viewall-components-masthead
https://jgiardino.github.io/patternfly-atomic/?p=viewall-components-navigation

Updates include:

  • included skip to main link
  • created separate components for nav, navlink, and utilities nav
  • documentation and accessibility notes for masthead
  • accessibility updates for masthead

Still to do:

  • write documentation for nav, navlink and utilities nav

Questions:

  • Where/how should the skip to main link display? Where should it display for smaller screensizes? Is it even needed when the navigation is hidden and the bars icon displays?
  • Keyboard access doesn't seem to work at all in patternlab components on Firefox.
  • Why do the images not display on my github page? They display locally

…ock with conditional icon so that link text can be used as aria-label value for dropdown
Copy link
Member

@andresgalante andresgalante left a comment

Choose a reason for hiding this comment

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

@jgiardino I am sure sure I am missing the point but I don't see the point of navlink been a thing by itself, why not adding it under the nav umbrella?

Also, tabs is a nav with a modifier class, so maybe we should consider putting tabs under navigation also like bootstrap does. If this increases the scope of the story and you don't have time to do it, then I can continue with it.

@@ -0,0 +1,18 @@
<ul class="nav {{ulClass}}">
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 ulClass and the other variables referring a type selector are too general. Is this a case to use styleModifier? #51

If it is I can ask Brian to fix our implementation so we can use it.

}
}

.pf-masthead__skip {
Copy link
Member

Choose a reason for hiding this comment

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

the skip link is creating horizontal scrolling

Copy link
Author

Choose a reason for hiding this comment

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

I can't believe I missed that. bitmoji

@andresgalante
Copy link
Member

@jgiardino sorry for not been able to review this :( it's been a month!

I don't really understand navlink.hbs but I'll take a closer look during the weekend and merge it early next week.

Thanks a lot!

@andresgalante andresgalante merged commit 6834bc4 into patternfly:master Jul 6, 2017
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.

2 participants