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

Headings: accessibility, hierarchy, and cosmetics of in-line `code` therein #15915

Closed
wants to merge 13 commits into from

Conversation

Projects
None yet
8 participants
@StevenBlack
Copy link
Contributor

StevenBlack commented Feb 25, 2015

Two changes in the javascript button documentation, methods section: structural and cosmetic.

Before we had a h2 - h4 hierarchy. The h4 was javascript code.

Changing to a h2 - h3 hierarchy looks kind of wonky since the h3 is code, and it heads a single-line explanation.

After — in this pull request — we have an h2 with a definition list.

Note I added a p tag on the dd elements to get a better looking bottom margin between dt elements in the list.

bs-button

@mdo

This comment has been minimized.

Copy link
Member

mdo commented Feb 25, 2015

We'd probably want the same styles on all our code headings then, no? We have those in a lot of places.

@StevenBlack

This comment has been minimized.

Copy link
Contributor Author

StevenBlack commented Feb 25, 2015

I count 29 other instances, yes. If you prefer code in definition lists rather than headings (my guess: yes) then I'm on it.

$ ack "h4.*\("

alerts.html
39:  <h4>$().alert()</h4>
42:  <h4>$().alert('close')</h4>

carousel.html
198:  <h4>.carousel(options)</h4>
206:  <h4>.carousel('cycle')</h4>
209:  <h4>.carousel('pause')</h4>
213:  <h4>.carousel(number)</h4>
216:  <h4>.carousel('prev')</h4>
219:  <h4>.carousel('next')</h4>

collapse.html
223:  <h4>.collapse(options)</h4>
231:  <h4>.collapse('toggle')</h4>
234:  <h4>.collapse('show')</h4>
237:  <h4>.collapse('hide')</h4>

dropdowns.html
162:  <h4>$().dropdown('toggle')</h4>

modal.html
459:  <h4>.modal(options)</h4>
467:  <h4>.modal('toggle')</h4>
471:  <h4>.modal('show')</h4>
475:  <h4>.modal('hide')</h4>

popovers.html
251:  <h4>$().popover(options)</h4>
254:  <h4>.popover('show')</h4>
258:  <h4>.popover('hide')</h4>
262:  <h4>.popover('toggle')</h4>
266:  <h4>.popover('destroy')</h4>

scrollspy.html
97:  <h4>.scrollspy('refresh')</h4>

tabs.html
95:  <h4>$().tab</h4>

tooltips.html
212:  <h4>$().tooltip(options)</h4>
215:  <h4>.tooltip('show')</h4>
219:  <h4>.tooltip('hide')</h4>
223:  <h4>.tooltip('toggle')</h4>
227:  <h4>.tooltip('destroy')</h4>    
@StevenBlack

This comment has been minimized.

Copy link
Contributor Author

StevenBlack commented Feb 25, 2015

OK I hit all 29 instances. The general case has more text than buttons.html so I reverted buttons.html back to using h4 tags. Starting now all h4 tags have code tags where appropriate.

Here's what it looks like now, using modal.html here as an example.

bs-modal

@patrickhlauke

This comment has been minimized.

Copy link
Member

patrickhlauke commented Feb 25, 2015

LGTM

@hnrch02 hnrch02 added this to the v3.3.4 milestone Feb 25, 2015

@hnrch02

This comment has been minimized.

Copy link
Member

hnrch02 commented Feb 25, 2015

@mdo You down with this?

@StevenBlack

This comment has been minimized.

Copy link
Contributor Author

StevenBlack commented Feb 27, 2015

Slightly larger for you. I think this looks really good.

codeinh4-2

Covering all instances of in-line code in headings
As a result of #15946 there are other instances in documentation where we have in-line code in headings.  Fixed.
@StevenBlack

This comment has been minimized.

Copy link
Contributor Author

StevenBlack commented Feb 27, 2015

The CSS commit above covers some instances of code-in-headings not seen in the components section of the docs.

Here's a before-and-after look at code in larger headings. This is code in-line with other heading text. Note: in-line code in headings inherit the heading background color.

bs-code-headings

@patrickhlauke

This comment has been minimized.

Copy link
Member

patrickhlauke commented Feb 28, 2015

LGTM as well. Still waiting for @mdo's blessing :)

@StevenBlack StevenBlack changed the title Accessibility and look changes to headings (button.html) Headings: accessibility, hierarchy, and cosmetics of in-line `code` therein Feb 28, 2015

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Mar 2, 2015

👍 from me too

@cvrebert

This comment has been minimized.

Copy link
Member

cvrebert commented Mar 2, 2015

I am very much in favor of using <code> for method headings in the API docs like this.

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Mar 2, 2015

@StevenBlack: can you fetch and rebase?

@@ -28,6 +28,11 @@ body {
font-weight: normal;
}

/* In-line code within headings retain the heading background */

This comment has been minimized.

Copy link
@XhmikosR

XhmikosR Mar 2, 2015

Member

It's inline and not in-line as far as I can tell.

Also, break one selector per line.

This comment has been minimized.

Copy link
@StevenBlack

StevenBlack Mar 2, 2015

Author Contributor

@XhmikosR Confirming: in-line is correct usage.

This comment has been minimized.

Copy link
@XhmikosR

XhmikosR Mar 2, 2015

Member

I'd rather have it inline.

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Mar 2, 2015

@StevenBlack: Also squash the patches and take care of my previous comment please.

patrickhlauke and others added some commits Mar 2, 2015

Covering all instances of in-line code in headings
As a result of #15946 there are other instances in documentation where we have in-line code in headings.  Fixed.
Merge branch 'docs.js.button' of https://github.com/StevenBlack/boots…
…trap into docs.js.button

* 'docs.js.button' of https://github.com/StevenBlack/bootstrap:
  Covering all instances of in-line code in headings
  Code in headings retain heading background color
  Added code tags to method sub-headings
  Accessibility and cosmetic changes to headings

Conflicts:
	docs/_includes/js/popovers.html
	docs/_includes/js/tooltips.html
	docs/assets/css/src/docs.css
@StevenBlack

This comment has been minimized.

Copy link
Contributor Author

StevenBlack commented Mar 2, 2015

Having no success squashing.

@kkirsche

This comment has been minimized.

Copy link
Contributor

kkirsche commented Mar 2, 2015

@StevenBlack git rebase -i HEAD~13 [your-branch-name]

You'll be presented with a list of commits that say pick [something] "commit message".

Change all except the first pick with squash

Then do a git push with (I believe) git push origin [your-branch-name] --force

@cvrebert

This comment has been minimized.

Copy link
Member

cvrebert commented Mar 2, 2015

Looks like the rebase had some problems. I'm happy to do the squashing when merging this. Just waiting on @mdo to approve the style changes.

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Mar 2, 2015

Don't specify the number of commits. Just run git rebase --i
upstream/master after you have fetched upstream.
On Mar 2, 2015 10:43 PM, "Kevin Kirsche" notifications@github.com wrote:

@StevenBlack https://github.com/StevenBlack git rebase -i HEAD~13
[your-branch-name

You'll be presented with a list of commits that say pick [something]
"commit message".

Change all except the first pick with squash


Reply to this email directly or view it on GitHub
#15915 (comment).

@StevenBlack

This comment has been minimized.

Copy link
Contributor Author

StevenBlack commented Mar 2, 2015

And we do this busy-work because....? Just curious.

@XhmikosR

This comment has been minimized.

Copy link
Member

XhmikosR commented Mar 2, 2015

Because it's useless to have many patches for the same thing apart from
making git history bigger. Also, it's sometimes mandatory to rebase anyway
due to conflicts.
On Mar 2, 2015 10:55 PM, "Steven Black" notifications@github.com wrote:

And we do this busy-work because....? Just curious.


Reply to this email directly or view it on GitHub
#15915 (comment).

@kkirsche

This comment has been minimized.

Copy link
Contributor

kkirsche commented Mar 2, 2015

Ah. Sorry @XhmikosR I've had bad luck doing it that way. As long as it works though in the end that's what matters.

But yeah, @StevenBlack having a single commit to point to for a patch, in my opinion, can make it easier to identify where a regression or bug was introduced and trace it back to the correct issue so you don't have 15+ commits for a single patch.

Just my opinion though 😃

@XhmikosR XhmikosR closed this in 4578850 Mar 3, 2015

@cvrebert cvrebert referenced this pull request Mar 3, 2015

Closed

v3.3.4 ship list #15624

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.