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

fixes #4609 - Inconsistent font size of codeblocks in titles of markdown documents. #4617

Merged
merged 4 commits into from May 27, 2018

Conversation

@jirivrany
Copy link
Contributor

@jirivrany jirivrany commented May 22, 2018

No description provided.

@jasongrout jasongrout added this to the Beta 3 milestone May 22, 2018
Copy link
Contributor

@jzf2101 jzf2101 left a comment

LGTM

@jzf2101
Copy link
Contributor

@jzf2101 jzf2101 commented May 24, 2018

@jasongrout @tgeorgeux do either of you have any thoughts before we merge?

Copy link
Contributor

@gnestor gnestor left a comment

It would be better to combine these rules into one:

.jp-RenderedHTMLCommon pre,
.jp-RenderedHTMLCommon code {
  font-size: unset;
}

Additionally, We may want to consider using font-size: inherit. According to the definition:

unset: This keyword applies the inherited value if the property is normally inherited (such as color), or the initial value if the property is normally not inherited (such as display).

I don't think we want to use the initial value in this context, just the inherit value which is the font-size of the parent (in this case, the heading).

@jirivrany
Copy link
Contributor Author

@jirivrany jirivrany commented May 24, 2018

Thanks @gnestor, you are right the inherit is a better option.

Currently, there is a following set of rules for given selector combination pre and code

.jp-RenderedHTMLCommon pre,
.jp-RenderedHTMLCommon code {
  border: 0;
  background-color: var(--jp-layout-color0);
  color: var(--jp-content-font-color1);
  font-family: var(--jp-code-font-family);
  font-size: var(--jp-code-font-size);
  line-height: var(--jp-code-line-height);
  padding: 0px;
}

And so It can be changed to:

.jp-RenderedHTMLCommon pre,
.jp-RenderedHTMLCommon code {
  border: 0;
  background-color: var(--jp-layout-color0);
  color: var(--jp-content-font-color1);
  font-family: var(--jp-code-font-family);
  font-size: inherit;
  line-height: var(--jp-code-line-height);
  padding: 0px;
}

I did a test with normal code block and the font-size looks fine.
screen_fixed

@gnestor
Copy link
Contributor

@gnestor gnestor commented May 24, 2018

Looks good @jirivrany 👍

I'm restarting the Appveyor build because it looks like it failed due to an unrelated error

Copy link
Contributor

@tgeorgeux tgeorgeux left a comment

I installed and ran it, seems to work great. Thanks for working on this @jirivrany !

@jirivrany
Copy link
Contributor Author

@jirivrany jirivrany commented May 25, 2018

Thank you @tgeorgeux I'm glad to help. Is there anything that I can do with the failing CI @gnestor ? Integrity check on my machine is ok and locals test are passing too.

@gnestor
Copy link
Contributor

@gnestor gnestor commented May 25, 2018

We are experiencing an issue with Appveyor that's affecting all builds right now. I restarted the Travis build because that looks like it's unrelated.

@gnestor
Copy link
Contributor

@gnestor gnestor commented May 25, 2018

@jirivrany We just fixed the Appveyor issue. Can you rebase from current master and push?

@gnestor gnestor force-pushed the font-size-codeblocks-titles branch from 02d4aa3 to 5d1074c May 27, 2018
@gnestor
Copy link
Contributor

@gnestor gnestor commented May 27, 2018

@jirivrany I just fixed the rebase 👍

@jirivrany
Copy link
Contributor Author

@jirivrany jirivrany commented May 27, 2018

@gnestor thank you. I'm not very experienced with rebasing, usually I use only normal merge. I used following commands:

git rebase upstream/master
git pull 
git push

What is the correct sequence? For the next time.

@jzf2101
Copy link
Contributor

@jzf2101 jzf2101 commented May 27, 2018

Are we ready to merge?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 27, 2018

Yep, pending CI we should be good.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 27, 2018

@jirivrany A rebase typically goes like:

git checkout master
git pull upstream master
git checkout feature-branch
git rebase master

This replays the commits on feature-branch on top of master. If you are lucky there will not be any conflicts. If there are conflicts, you will need to resolve them by hand.

@jirivrany
Copy link
Contributor Author

@jirivrany jirivrany commented May 27, 2018

Thanks @ian-r-rose - will do it next time.

@jzf2101 jzf2101 merged commit e423106 into jupyterlab:master May 27, 2018
2 checks passed
@jzf2101
Copy link
Contributor

@jzf2101 jzf2101 commented May 27, 2018

Thx again @jirivrany

@jirivrany jirivrany deleted the font-size-codeblocks-titles branch May 28, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants