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
Closed

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

wants to merge 13 commits into from

Conversation

StevenBlack
Copy link
Contributor

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
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

LGTM

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

hnrch02 commented Feb 25, 2015

@mdo You down with this?

@StevenBlack
Copy link
Contributor Author

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

codeinh4-2

As a result of #15946 there are other instances in documentation where we have in-line code in headings.  Fixed.
@StevenBlack
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

XhmikosR commented Mar 2, 2015

👍 from me too

@cvrebert
Copy link
Collaborator

cvrebert commented Mar 2, 2015

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

@XhmikosR
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 */
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 inline and not in-line as far as I can tell.

Also, break one selector per line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@XhmikosR Confirming: in-line is correct usage.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather have it inline.

@XhmikosR
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 9 commits March 2, 2015 14:42
As a result of #15946 there are other instances in documentation where we have in-line code in headings.  Fixed.
…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
Copy link
Contributor Author

Having no success squashing.

@kkirsche
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
Copy link
Collaborator

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
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
Copy link
Contributor Author

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

@XhmikosR
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
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 mentioned this pull request Mar 3, 2015
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.

None yet

8 participants