Refactor underline nav

[ampinsk]

This refactors the underline nav component and and adds documentation to the Navigation component. As discussed here, this:

• keeps orange 2px underline, makes selected state bold
• introduces hover state
• reduces horizontal padding
• refactors Sass to work in all contexts (profiles, search, and up-coming teams update)
• brings class names in alignment with component naming convention
• moves into Primer and adds documentation

/cc @primer/ds-core

[jonrohan]

👍 from me.

I went through this PR and changed our stories to use storiesFromMarkdown. I also fixed the function to convert any octicons rails tags to a react component.

[broccolini]

@ampinsk We need to test this in github/github before shipping. @jonrohan did you two go through that process yet? If so let's get on that today so that we can include this in v10.

[broccolini]

This is looking great! I have some minor feedback on class names and a couple of nested styles, and a bit more feedback on documentation, but the majority of this looks 👌 ✨

Sorry this ended up being so late today, hopefully it won't be too hard to update class names in your github pr but let me know if you need a hang with that.

[broccolini]

I feel a little worried having UnderlineNav-items and UnderlineNav-item will be a bit confusing and not immediately obvious what they do. Could we change UnderlineNav-items to UnderlineNav-body? That's a pattern we use elsewhere and I think it makes sense because you have actions to handle other content. Otherwise best I could think of was UnderlineNav-items-container or wrapper but they get a little long 😬

[ampinsk]

Changing to UnderlineNav-body 👍

[broccolini]

I'd prefer to avoid nesting other component selectors like this because it creates dependencies and means overriding if we want to act differently. We could just use the ml-1 utility. How strongly do you feel this needs to be built into the pattern?

[ampinsk]

I removed this from the component. My only concern is that people will use inconsistent spacing, but I think it needs to be flexible because markup spacing affects this

[broccolini]

Yeah this, and the other similar scenarios, are tricky. This is where I feel like we should handle the Counter styling in context with the UnderlineNav in other parts of the code (say helpers or something), rather than Sass. Handling it in the Sass means adding additional classes to apply to components that may already require a number of classes. That's not the worst but can feel like overkill and be a contributor to CSS bloat. We don't have a good solution for handling elsewhere in the code base right now, but it keeps coming up and is something that's on our radar. If we start to see this particular use case being styled incorrectly, then the best option is to have another class to apply to the Counter rather than nesting it.

[broccolini]

Similar thoughts here to nesting Counter. We could have an element class to account for it, such as UnderlineNav-octicon as utilities won't work because you need the hover styles.

[ampinsk]

I removed the spacing from the component (same concern as Counter) but kept the color styles in under .UnderlineNav-octicon

[broccolini]

Same here re nesting other Primer styles. UnderlineNav-container may work? It's a little bit of a quirky one and potentially could be done with utilities but I can image this tripping people up so probably good to offer it as part of the component styles.

[ampinsk]

Changed this to .UnderlineNav-container 👍

[broccolini]

This could be worded better, it says navigation twice and descriptions could be a little more helpful. Suggest something like this but feel free to edit:

Use UnderlineNav to style navigation with a minimal underlined selected state. This component comes with variations to accommodate icons, counters, and button actions; and is typically used for navigation placed at the top of the page.

[broccolini]

Use is used twice here, could be worded better, suggest something like this:

Use .UnderlineNav-actions to place another element, such as a button, to the opposite side of the navigation items.

[broccolini]

To keep in style with our newer docs (i.e. less wordy and command tense), suggest changing this to:

Use .UnderlineNav--right to right align the navigation.

[broccolini]

Suggest changing description to: UnderlineNav--right also works with when used with UnderlineNav-actions.

[broccolini]

Again, use a more commanding tone, such as:

Counters and octicons can be used with navigation items:

[broccolini]

Suggesting change wording to something like:

Use UnderlineNav--full in combination with container styles to make navigation fill the width of the container.

I couldn't find a way to avoid using "container" twice but I think it's okay.

[ampinsk]

@broccolini this is ready for review again! ✨

[broccolini]

I just realized we need to remove that it accommodates counters becaase of the Sass changes 😬

[ampinsk]

I changed it to:

Use .UnderlineNav to style navigation with a minimal underlined selected state, typically used for navigation placed at the top of the page. This component comes with variations to accommodate icons, containers, and other content.

It's a little wordy, so let me know if you have suggestions!

[broccolini]

Yeah this, and the other similar scenarios, are tricky. This is where I feel like we should handle the Counter styling in context with the UnderlineNav in other parts of the code (say helpers or something), rather than Sass. Handling it in the Sass means adding additional classes to apply to components that may already require a number of classes. That's not the worst but can feel like overkill and be a contributor to CSS bloat. We don't have a good solution for handling elsewhere in the code base right now, but it keeps coming up and is something that's on our radar. If we start to see this particular use case being styled incorrectly, then the best option is to have another class to apply to the Counter rather than nesting it.

[broccolini]

This looks great! Thank you so much for working on this! 💖 I just have one small comment, otherwise this looks good to merge