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

Fix task list rendering (Fix #749) #757

Merged
merged 1 commit into from
Jan 30, 2019
Merged

Fix task list rendering (Fix #749) #757

merged 1 commit into from
Jan 30, 2019

Conversation

jhildenbiddle
Copy link
Member

This PR restores the task list presentation removed in 4.8:

  • Add “task-list-item” class to task list <li> elms
  • Hide list bullets on unordered task lists <li> elms

It also provides several improvements on the pre-4.8 presentation:

  • Add “task-list” class to task list <ul> elms
  • Display list numbers on ordered task lists <li> elms
  • Render accessible task list items by wrapping checkbox and text in <label> elm
  • Allow task lists to be nested within standard ordered/unordered lists

Please makes sure these boxes are checked before submitting your PR, thank you!

  • Make sure you are merging your commits to master branch.
  • Add some descriptions and refer relative issues for you PR.
  • DO NOT include files inside lib directory.

- Add “task-list” class to task list `<ul>` elms
- Add “task-list-item” class to task list `<li>` elms
- Hide list bullets on unordered task lists `<li>` elms
- Display list numbers on ordered task lists `<li>` elms
- Render accessible task list items by wrapping checkbox and text in `<label>` elm
@jhildenbiddle
Copy link
Member Author

@QingWei-Li — I suspect this is a general Travis issue and not an actual docsofy build issue. Can we rerun CI to clear this PR for a merge?

@QingWei-Li QingWei-Li merged commit 69ef489 into docsifyjs:master Jan 30, 2019
@jhildenbiddle
Copy link
Member Author

Excellent!

Thanks, @QingWei-Li.

@timaschew
Copy link
Member

Let's create a follow up PR to provide some tests for this feature.

@jhildenbiddle
Copy link
Member Author

@timaschew -- agreed on the need for automated tests, but since docsify has no test framework in place I think testing is a larger conversation to be handled separately.

Let's move the discussion to #386 and decide how to move forward.

@timaschew
Copy link
Member

Actually, I've created it: #760
It's not End2end but I think this is better than nothing.

@jhildenbiddle
Copy link
Member Author

Awesome! Didn't see that PR. Thanks for the follow-up, @timaschew!

@timaschew
Copy link
Member

ping @jhildenbiddle

@jhildenbiddle
Copy link
Member Author

Whats up, @timaschew?

@jhildenbiddle jhildenbiddle deleted the 749 branch February 13, 2019 19:17
@timaschew
Copy link
Member

@jhildenbiddle
Copy link
Member Author

Weird. I'm not seeing any comments by following the link or manually navigating to the "Files Changed" tab. Maybe they aren't showing because the PR has already been merged?

Since I can't see the comment I don't know what the concern is, but if it has to do with the argument name I thought I'd mention that I'm using the same argument names listed on marked.js.org (see "Block level renderer methods" > "list(string body, boolean ordered, number start)").

@timaschew
Copy link
Member

That's really weird, it's even shown to me on this page, here is a screenshot:
screen shot 2019-02-13 at 21 02 13

@jhildenbiddle
Copy link
Member Author

Weird indeed. Attached is a full-page screenshot from my view:

wtf

Anyway, the start argument allows an ordered list to begin with a number other than 1. A sample block of markdown you can use to test is as follows:

1. One
2. Two

Text

3. Three

Docsify should render the following HTML:

<ol>
  <li>One</li>
  <li>Two</li>
</ol>
<p>Text</p>
<ol start="3">
  <li>Three</li>
</ol>

Notice the start attribute on the second <ol> element.

@timaschew
Copy link
Member

Ah, got it, thanks for clarification!
I've extended the tests.

@jhildenbiddle
Copy link
Member Author

Excellent. Thanks for taking on the testing task, @timaschew. Hopefully we can get others to follow suit.

QingWei-Li pushed a commit that referenced this pull request Feb 19, 2019
This PR restores the task list presentation removed in 4.8:

- Add “task-list-item” class to task list `<li>` elms
- Hide list bullets on unordered task lists `<li>` elms

It also provides several improvements on the pre-4.8 presentation:

- Add “task-list” class to task list `<ul>` elms 
- Display list numbers on ordered task lists `<li>` elms
- Render accessible task list items by wrapping checkbox and text in `<label>` elm
- Allow task lists to be nested within standard ordered/unordered lists 

Please makes sure these boxes are checked before submitting your PR, thank you!

* [x] Make sure you are merging your commits to `master` branch.
* [x] Add some descriptions and refer relative issues for you PR.
* [x] DO NOT include files inside `lib` directory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants