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

[geom_series] remark_admonition #493

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

longye-tian
Copy link
Collaborator

@longye-tian longye-tian commented Jul 1, 2024

Dear John @jstac, Matt @mmcky

This pull request is for #442.

I reviewed all current QuantEcon intro lecture series. There are four lectures that contain remarks. Here is a summary:

  • geom_series.md
  • money_inflation.md
  • unpleasant.md
  • money_inflation_nonlinear.md

This pull request uses the geom_series.md lecture as an example to test the current admonition style.

Best ❤️
Longye

Dear John,

This pull request is for #442.

I use the geom_series.md lecture as an example to test the current admonition style.

Best,
Longye
@longye-tian longye-tian requested a review from mmcky July 1, 2024 00:19
Copy link

netlify bot commented Jul 1, 2024

Deploy Preview for taupe-gaufre-c4e660 ready!

Name Link
🔨 Latest commit c281cdb
🔍 Latest deploy log https://app.netlify.com/sites/taupe-gaufre-c4e660/deploys/66e3f14ad9bccf0008c9888f
😎 Deploy Preview https://deploy-preview-493--taupe-gaufre-c4e660.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mmcky
Copy link
Contributor

mmcky commented Jul 1, 2024

thanks @longye-tian this will be a big help. We can work on updating sphinx-proof styles.

Are you familiar with any html/css? If you are we could link up and I can show you where to submit a PR with suggested style updates. But no worries if not.

@longye-tian
Copy link
Collaborator Author

thanks @longye-tian this will be a big help. We can work on updating sphinx-proof styles.

Are you familiar with any html/css? If you are we could link up and I can show you where to submit a PR with suggested style updates. But no worries if not.

Hi Matt,

No worries. I'm not familiar with the html and css.

Best,
Longye

Copy link

github-actions bot commented Jul 1, 2024

@github-actions github-actions bot temporarily deployed to pull request July 1, 2024 00:26 Inactive
@github-actions github-actions bot temporarily deployed to pull request July 1, 2024 00:27 Inactive
@mmcky
Copy link
Contributor

mmcky commented Jul 1, 2024

thanks @longye-tian.

This is what the current styling is.
Screenshot 2024-07-01 at 10 31 42 AM

@jstac I propose that sphinx-proof adopts just a thin colour bar on the left hand side of the block.
Any thoughts or recommendations in colour choices to represent remark?

@mmcky mmcky changed the title [geom_series] remark_admonition [sphinx-proof style review][geom_series] remark_admonition Jul 1, 2024
@mmcky mmcky added the in-work label Jul 1, 2024
@jstac
Copy link
Contributor

jstac commented Jul 1, 2024

Any thoughts or recommendations in colour choices to represent remark

How about green similar to the numbers in

image

@mmcky
Copy link
Contributor

mmcky commented Jul 2, 2024

@jstac is this closer to what you're thinking?

Screenshot 2024-07-02 at 3 38 56 PM

I will see if I can match the icon colour to the line colour as well.

I have setup executablebooks/sphinx-proof#106 to do a full review so that we can make all admonitions follow a similar minimal styling approach. I will need to define unique colours for all types of admonitions and style them all by a thin LHS line.

@jstac
Copy link
Contributor

jstac commented Jul 2, 2024

Looks great @mmcky , many thanks.

@mmcky
Copy link
Contributor

mmcky commented Jul 11, 2024

@mmcky
Copy link
Contributor

mmcky commented Jul 15, 2024

  • test improvements to sphinx-proof package

@github-actions github-actions bot temporarily deployed to pull request July 15, 2024 23:05 Inactive
@mmcky
Copy link
Contributor

mmcky commented Jul 15, 2024

@DrDrij can I get your advise here.

I have just updated sphinx-proof with new styling around the sphinx-proof admonitions, however it looks like quantecon-book-theme is overriding the title style with a background colour to show

Screenshot 2024-07-16 at 9 14 08 AM

instead of

image

Our theme has

  .admonition {
    font-size: 0.9rem;
    margin: 1.5rem auto;
    padding: 0 1rem 0.5rem 1rem;
    page-break-inside: avoid;
    box-shadow: 0 0.2rem 0.5rem rgba(0, 0, 0, 0.1),
      0 0 0.05rem rgba(0, 0, 0, 0.1);

    .admonition-title {
      position: relative;
      margin: 0 -1rem;
      padding: 0.25rem 2rem;
      font-weight: 700;
      background-color: #0072bc26;
    }
  }

and it makes sense for it to be the main "style" here but it is quite general.

Is the best solution here to update sphinx-proof to pre-pend an admonition name that is less generic so instead of using admonition-title we should use proof-admonition-title. I guess the downside of this is that it becomes harder for "themes" to override the style.

Otherwise, we could update our own theme to use a transparent background for admonition-title for consistency across the lecture.

@DrDrij
Copy link
Member

DrDrij commented Jul 18, 2024

@mmcky Hey Matt I notice that the admonition has the proof class <div class="proof remark admonition" id="remark-0">.

So we could style it specifically such as:

div.remark.proof {
   border-color: green;
   .admonition-title {
     background-color: transparent;
   }
 }

Not sure if I have answered your question correctly though. :)

@mmcky
Copy link
Contributor

mmcky commented Jul 18, 2024

thanks @DrDrij so this is what we have in sphinx-proof

/*********************************************
* Remark *
*********************************************/
div.remark {
	border-color: var(--remark-border-color);
	background-color: none;
}

div.remark p.admonition-title {
	background-color: transparent;
}

but the title in this preview has a background colour that I think is coming from quantecon-book-theme?

maybe transaparent is allowing the quantecon-book-theme underneath?

@DrDrij
Copy link
Member

DrDrij commented Jul 19, 2024

Ah I see! Yes I made a mistake.

div.remark p.admonition-title {
	background-color: transparent;
}

Will override the CSS I wrote above. CSS weighting is sequentially - element, id, class, then order. So I am adjusting the CSS I wrote from:

div.remark.proof {
   border-color: green;
   .admonition-title {
     background-color: transparent;
   }
 }

To:

div.remark.proof {
   border-color: green;
   p.admonition-title {
     background-color: transparent;
   }
 }

This way it will override regardless of order loaded on client side since it has 1 more class of specificity.

@mmcky
Copy link
Contributor

mmcky commented Jul 19, 2024

@DrDrij my target is to get no background colour -- as a minimal theme supplied by sphinx-proof. I used transparent so it would match the background colour of the page but it looks like the blue background is coming through from quantecon-book-theme instead.

@github-actions github-actions bot temporarily deployed to pull request July 22, 2024 00:14 Inactive
@mmcky
Copy link
Contributor

mmcky commented Jul 22, 2024

@DrDrij I made the suggested change in executablebooks/sphinx-proof#109 but it is still showing the blue background from quantecon-book-theme. Not sure how to override that style.

Screenshot 2024-07-22 at 10 15 06 AM

@mmcky mmcky removed the ready label Jul 23, 2024
DrDrij added a commit to executablebooks/sphinx-proof that referenced this pull request Jul 29, 2024
This CSS tweak adjust the styling so that styles can be overriden without having to be forced. 

They will cascade from py_data -> quantecon_book_theme -> sphinx_proof.

QuantEcon/lecture-python-intro#493
@DrDrij
Copy link
Member

DrDrij commented Jul 29, 2024

@mmcky I hope this is a reasonable request to make small changes to quantecon-book-theme and sphinx-proof to allow the styling of the admonition title background without having to force styles (using !important). I think it is best to do it properly.

executablebooks/sphinx-proof#111

QuantEcon/quantecon-book-theme#248

image

So what these changes involve is having each stylesheet (pydata, qe theme, and sphinx proof) all style the admonition title the same way. This means the ultimate background colour style will come from whichever stylesheet is loaded last in the chain. Which looks like proof.css.

image

@github-actions github-actions bot temporarily deployed to pull request August 20, 2024 00:23 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 20, 2024 00:51 Inactive
@mmcky
Copy link
Contributor

mmcky commented Aug 20, 2024

love your work @DrDrij

Screenshot 2024-08-20 at 11 28 32 AM

I will make new releases of:

  • quantecon-book-theme, and
  • sphinx-proof

then we will have minimal styles for all those admonitions such as remark.

mmcky pushed a commit to executablebooks/sphinx-proof that referenced this pull request Sep 8, 2024
This CSS tweak adjust the styling so that styles can be overriden without having to be forced. 

They will cascade from py_data -> quantecon_book_theme -> sphinx_proof.

QuantEcon/lecture-python-intro#493
@github-actions github-actions bot temporarily deployed to pull request September 10, 2024 03:58 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 11, 2024 00:13 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 11, 2024 00:58 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 11, 2024 00:59 Inactive
@mmcky mmcky changed the title [sphinx-proof style review][geom_series] remark_admonition [geom_series] remark_admonition Sep 11, 2024
@mmcky
Copy link
Contributor

mmcky commented Sep 11, 2024

thanks @longye-tian -- Would you mind to update the other lectures with remark admonitions and then we can merge this PR.

Add remark admonition for money_inflation.md
Add remark admonition to unpleasent.md
Add remark admonition for money_inflation_nonlinear.md
@longye-tian
Copy link
Collaborator Author

thanks @longye-tian -- Would you mind to update the other lectures with remark admonitions and then we can merge this PR.

Hi Matt @mmcky ,

I have updated the remark admonitions for these lectures.
I'll have a look on other lectures to see if there is any new lectures that need remark admonitions.

Best,
Longye

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants