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

#24373 : Documentation/Navbar : Refer to utility classes used. Hidden when printing. #24380

Merged
merged 11 commits into from
Oct 18, 2017
Merged

#24373 : Documentation/Navbar : Refer to utility classes used. Hidden when printing. #24380

merged 11 commits into from
Oct 18, 2017

Conversation

aavmurphy
Copy link
Contributor

@aavmurphy aavmurphy commented Oct 15, 2017

refer to the (not navbar) classes used for newbies
add: navbars don't print
give the 1st example a drop-down (as its the template people will copy and adapt)

refer to (not navbar) classes used for newbies
navbars don't print, so add example of a print-only alternative
typo in {% example %}
add links to component pages
Here's an example of all the sub-components included in a responsive light-themed navbar that automatically collapses at the `lg` (large) breakpoint.
Here's an example of all the sub-components.
- the navbar is light-themed
- its responsive - it automatically collapses at the `lg` (large) breakpoint.
Copy link
Member

Choose a reason for hiding this comment

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

It's

I assume id="camelCase" is the standard for id names, rather than the clearly superior id="underscore_words"
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Rather than add an entire new section to this page, let's put this under the How it works list.

@@ -29,7 +29,10 @@ Navbars come with built-in support for a handful of sub-components. Choose from
- `.navbar-text` for adding vertically centered strings of text.
- `.collapse.navbar-collapse` for grouping and hiding navbar contents by a parent breakpoint.

Here's an example of all the sub-components included in a responsive light-themed navbar that automatically collapses at the `lg` (large) breakpoint.
Here's an example of all the sub-components.
Copy link
Member

Choose a reason for hiding this comment

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

Revert the changes to this section, and instead let's add a mention to the how it works list:

Use spacing and flexbox utility classes for sub-component alignment.

@@ -536,3 +550,16 @@ Sometimes you want to use the collapse plugin to trigger hidden content elsewher
</nav>
</div>
{% endexample %}

## Printing
Copy link
Member

Choose a reason for hiding this comment

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

This should also be a note in the how it works section. The rest of this isn't relevant to the navbar itself and can be covered by the display utilities docs.

Navbars are hidden by default when printing. Force them to be printed by adding .d-print to the .navbar.

still like to emphasise the foreign utility classes in the main example
bg-light's name is self explanatory (so its obvious where to look it up), but mr-sm-2's name isn't at first
@aavmurphy aavmurphy changed the title #24373 : Documentation/Navbar : Refer to utility classes uses. Add Printing section #24373 : Documentation/Navbar : Refer to utility classes used. Hidden when printing. Oct 16, 2017
... as they are position:fixed. 
better wording welcome
@aavmurphy
Copy link
Contributor Author

@mdo please re-check. have added 1 more thing (fixed-top/bottom uses position:fixed), so doesn't reserve a gap on the screen. couldn't thing of a nice way to phrase it though. thanks

- rewrite utils line and link to the utils pages
- restore the paragraphs to the placement section
- fix formatting and grammar
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

I pushed another commit to fine tune some things here around the language and formatting of the content—more links to utilities pages, better formatting of paragraphs and lists, etc.

Thanks for the back and forth!

@mdo mdo merged commit ad77ea7 into twbs:v4-dev Oct 18, 2017
@aavmurphy
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants