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.

Sign up for GitHub

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

Jump to bottom

refactor: refactor code tab split #1369

Merged
merged 3 commits into from
May 2, 2019
Merged

refactor: refactor code tab split #1369

merged 3 commits into from
May 2, 2019

Conversation

zachbadgett
Copy link
Contributor

@zachbadgett zachbadgett commented Apr 18, 2019 •
edited by endiliey
Loading

Motivation

Fix #1260 to allow html/xml comments in code tabs.
Unsure if this would be considered a refactor or a fix since it changes the way the tabs are parsed.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Only tested it locally, ensuring the code tabs on page /docs/en/doc-markdown still worked.
Would like to get the logic approved before getting too far into testing.
Use cases involving whitespace still need to be tested.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 18, 2019
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Apr 18, 2019 •
edited
Loading

Deploy preview for docusaurus-2 ready!

Built with commit 1a6897e

https://deploy-preview-1369--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit a1d7b9d

https://deploy-preview-1369--docusaurus-preview.netlify.com

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Apr 18, 2019 •
edited
Loading

Deploy preview for docusaurus-preview ready!

Built with commit 1a6897e

https://deploy-preview-1369--docusaurus-preview.netlify.com

endiliey
endiliey reviewed Apr 18, 2019
View reviewed changes
Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

This seems to be a fix that changes the prev implementation to be more bug-less.

I am currently away so wont be able to test this out and review it carefully, but in the meantime do you mind to add comment on several places so that future contributor can understand your code intention ?

Thank you for the attempt, by the way 😊

@zachbadgett
Copy link
Contributor Author

@endiliey Added comments

@endiliey endiliey self-assigned this Apr 23, 2019
endiliey
endiliey approved these changes Apr 23, 2019
View reviewed changes
Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review.

LGTM. Awesome 🎉. However, there might be some edge cases that we might miss.

Would like to get the logic approved before getting too far into testing.
Use cases involving whitespace still need to be tested.

Are you going to write some unit test ? We can create fixtures to test out splitTabsToTitleAndContent

@endiliey endiliey requested a review from yangshun April 23, 2019 15:48
yangshun
yangshun suggested changes Apr 24, 2019
View reviewed changes
Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Would be good to write tests for these. This is quite a brittle section.

@zachbadgett
Copy link
Contributor Author

Yeah, I'll create unit tests.

zachbadgett
zachbadgett commented Apr 25, 2019
View reviewed changes
.eslintignore
@@ -9,3 +9,4 @@ website/
scripts
packages/docusaurus-1.x/lib/core/metadata.js
packages/docusaurus-1.x/lib/core/MetadataBlog.js
packages/docusaurus-1.x/lib/core/__tests__/split-tab.test.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added because the lint fixer keeps removing @jest-environment jsdom from my test file. I'm sure there's a workaround so this is just temporary.

@zachbadgett
Copy link
Contributor Author

@endiliey @yangshun Added a quick unit test; let me know if looks good. If it does I'll add more.

zachbadgett
zachbadgett commented Apr 25, 2019
View reviewed changes
packages/docusaurus-1.x/lib/core/__tests__/split-tab.test.js Outdated
it('renders content correctly', () => {
const node = docPage().getDOMNode();
const firstContent = node.querySelector('[id$="-content-7"]').textContent;
expect("console.log('Hello, world!');\n").toEqual(firstContent);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be okay to match the generated html from the highlighter.

endiliey
endiliey reviewed Apr 25, 2019
View reviewed changes
packages/docusaurus-1.x/lib/core/__tests__/split-tab.test.js Outdated
};
beforeEach(() => {
props = {
content: `
Copy link
Contributor

@endiliey endiliey Apr 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

can we do content: fs.readFile('/some/path/to/fixtures/doc1.md') instead ?

This was referenced Apr 25, 2019
Merged
Closed
@zachbadgett zachbadgett force-pushed the code-tabs-comments-fix branch 3 times, most recently from 20ba2f2 to 4996207 Compare April 29, 2019 18:55
@zachbadgett
Copy link
Contributor Author

@endiliey Added fix for #1279

zachbadgett
zachbadgett commented Apr 29, 2019
View reviewed changes
packages/docusaurus-1.x/lib/core/Doc.js
const contents = m.split('\n').filter(c => {
if (!indents) {
indents = (
c.match(/((\t|\s{4})+)<!--DOCUSAURUS_CODE_TABS-->/) || []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can remove redundant parentheses.

@endiliey
Copy link
Contributor

I like the fact that you put test on it. So far LGTM, I will test it out again locally tonight. cc @yangshun

@endiliey
Copy link
Contributor

@zachbadgett

I just tried this out. This is awesome.

Great job on fixing #1260 🎉 . However, the #1279 will only works if it's written with indentation

For example, this does not work


1. Do this
<!--DOCUSAURUS_CODE_TABS-->
<!--tab1-->
```
some code
```
<!--tab2-->
```
some other code
```
<!--END_DOCUSAURUS_CODE_TABS-->
1. Do that

image

What do you think ?

@zachbadgett
Copy link
Contributor Author

@endiliey that might be an issue with remarkable. Adding a code block in between the list items on the online demo (https://jonschlinkert.github.io/remarkable/demo/) shows the same result.

@endiliey
Copy link
Contributor

endiliey commented May 2, 2019

Cool. Actually I don’t mind at all, I was just testing the expected behavior reported on #1279.

Anyway, this is already a huge improvement. Thank you very much.

cc @yangshun, looks good to you ? lgtm

@yangshun yangshun merged commit 0813920 into facebook:master May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@endiliey endiliey endiliey approved these changes

@yangshun yangshun yangshun approved these changes

Assignees

@endiliey endiliey

Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Code Tabs do not allow HMTL comments within code blocks
5 participants
@zachbadgett @docusaurus-bot @endiliey @yangshun @facebook-github-bot