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

[WIP] Fix #313 #330

Merged
merged 3 commits into from
May 14, 2019
Merged

[WIP] Fix #313 #330

merged 3 commits into from
May 14, 2019

Conversation

kurianbenoy
Copy link
Contributor

@kurianbenoy kurianbenoy commented May 13, 2019

@ivan I have been thinking about using css padding by calling a html element like this:

<div class="intent">
$ sudo yum update
$ sudo wget https://dvc.org/rpm/dvc.repo -O /etc/yum.repos.d/dvc.repo
 $ sudo yum install dvc
$ sudo yum update
$ sudo yum install dvc
</div>

with .intent having property padding-left:4em as css attribute.

I am not familiar with React, so I am not sure, what is correct change to be done there. Yet I hope this approach work, as its done for tag in docs.

Thanks

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

@kurianbenoy it should be done in a way that does not require inserting additional statements inside code blocks. Actually, we should remove extra leading whitespaces after this ticket is closed. I think that you would need to modify the src/Documentation/Markdown/Markdown.js line 61, CodeBlock renderer.

@kurianbenoy
Copy link
Contributor Author

@shcheklein I guess the change I made in src/Documentation/Markdown/Markdown.js in line 64 fixes the padding. Can I remove the 4 leading spaces in all documents in Get started section?

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

left a comment on how should we be applying the padding. Agreed on trying it on the get started first.

@kurianbenoy
Copy link
Contributor Author

I have incorporated the changes you suggested.

@shcheklein shcheklein temporarily deployed to dvc-org-pr-330 May 14, 2019 01:36 Inactive
@shcheklein
Copy link
Member

shcheklein commented May 14, 2019

@kurianbenoy check the deployment:

Screen Shot 2019-05-13 at 6 40 02 PM

It looks like 2em should be enough.

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Check the comments, check the deployment. It looks like 2em should be enough.

@shcheklein shcheklein temporarily deployed to dvc-org-pr-330 May 14, 2019 01:52 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-pr-330 May 14, 2019 01:55 Inactive
@kurianbenoy
Copy link
Contributor Author

Screenshot-2019-5-14 Data Science Version Control System(1)

@kurianbenoy
Copy link
Contributor Author

I feel it should have been better to use 3 or 4 em to be as aligned to current docs

@shcheklein
Copy link
Member

@kurianbenoy yep, 3em looks like a good option. We def don't need more.

@shcheklein
Copy link
Member

@kurianbenoy actually, even 2em is enough ) Up to you, 2em or 3em :)

@kurianbenoy
Copy link
Contributor Author

Ok let's stick to 2em then @shcheklein

@shcheklein
Copy link
Member

@algomaster99 @jorgeorpinel I'm going to merge this. Just keep in mind guys, that we don't need spaces any more. @algomaster99 when you run the prettier next time (if we can figure out all the caveats with it), please, make sure that it removes all spaces everywhere.

@kurianbenoy thank you! :)

@shcheklein shcheklein merged commit ab74826 into iterative:master May 14, 2019
@algomaster99
Copy link
Contributor

@shcheklein all the spaces should be removed inside code blocks know? There should be no indentation right?

@shcheklein
Copy link
Member

@algomaster99 correct! no need for the manual indentation anymore. @kurianbenoy already did this across the get started section. Would be great if prettier can handle everything else. Please, let us know.

@kurianbenoy let's first wait on @algomaster99 if it can be done automatically. If prettier can't handle this we can probably write a script do this across all .md files.

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