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

refactor(v2): improve markup of code blocks #2056

Closed
wants to merge 1 commit into from

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Nov 25, 2019

Motivation

  • replace pre with a div (as pre is already used in code blocks)
  • remove of excess wrapper
  • cleanup of unnecessary styles

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

See markup, no UI changes.

@lex111 lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Nov 25, 2019
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 25, 2019
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit 854f8a0

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

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit 854f8a0

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

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.

Seems that this improve things a little bit compared to #2048.

Can we somehow resolve the layout style ?

I kinda dislike the fact that there is a scrollbar now for https://deploy-preview-2056--docusaurus-2.netlify.com/docs/markdown-features

Its kinda zoomed and cutoff
image

Previously it was like this (before #2048 )
image

@lex111
Copy link
Contributor Author

lex111 commented Nov 26, 2019

Uh, yes, that’s bad :(
Although I have no idea how to fix it, but this PR does not affect it in any way.
@yangshun help please.

@yangshun
Copy link
Contributor

I will review in more detail over the Thanksgiving long weekend. The new version is not released yet so it can wait a bit. Thanks for catching it Endi!

@lex111
Copy link
Contributor Author

lex111 commented Nov 30, 2019

As I see the issue: we need to somehow limit the width of the container with block code, how to do this? This is not so difficult considering the following code, but it will not work because value is presented as a percentage :(

calc((var(--ifm-container-width) * var(--ifm-col-width)) / 100)
calc((1140px * 75%) / 100) // not work :(
calc((var(--ifm-container-width) * 75) / 100) // works!

Turns out you need to somehow get rid of the percent sign, but this is impossible (???), so the only option for me is to use hard-coded value (75 = parent col width), it's not very good at all, but I have no more ideas how to fix it. 😢

@endiliey
Copy link
Contributor

@lex111 I think we can revert #2048 with #2071 and then attempt to remove the autowrap again in new PR. What do you think ? Since we don't have a perfect solution yet.

I think its also very tied to infima variables css (that might change somehow). I have limited knowledge of infima although I have access to the repo, maybe @yangshun want to give @lex111 access to the repo?

@lex111
Copy link
Contributor Author

lex111 commented Nov 30, 2019

I actually was waiting for the @yangshun review these days, because this is a rather difficult case and there is probably a better solution.
I have access to Infima, but this is unlikely to help us.

@endiliey
Copy link
Contributor

Let's wait then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants