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 😬

Changing to UnderlineNav-body 👍

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?

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

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.

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.

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

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.

Changed this to .UnderlineNav-container 👍

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.

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.

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.

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

Again, use a more commanding tone, such as:

Counters and octicons can be used with navigation items:

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.

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

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!

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.