Skip to content

Conversation

@cderv
Copy link
Collaborator

@cderv cderv commented Jul 12, 2021

This reverts commit dc87225 (#1187) following discussion in #1040

Plan is to keep the scrolling but have the icon stays on the top right side even when scrolling.

The button icon will always show so there could be overlap with long line as we no more wrapp. However, the icon is a lighter option than a button, and by default there is an opacity filter. It will be darker on button hover only.

Demo here: https://cderv.github.io/bs4booktesting/with-scroll/

@cderv cderv requested a review from apreshill July 12, 2021 16:49
Copy link
Contributor

@apreshill apreshill left a comment

Choose a reason for hiding this comment

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

This looks good. Two minor suggestions:

  1. I don't think the margins are needed here anymore now (red background color added here to help visualize):
    Screen Shot 2021-07-12 at 11 43 43 AM

  2. I have a minor preference for 50% opacity 😎

    Screen.Recording.2021-07-12.at.11.44.18.AM.mov

@cderv
Copy link
Collaborator Author

cderv commented Jul 12, 2021

Ok for the 50%. It looks ok !

For the margin around the button I thought I removed them. I'll look into it. Maybe bootstrap add some by default we need to reduce.
Does it cause any issue in layout having the red margins above ?

@apreshill
Copy link
Contributor

apreshill commented Jul 12, 2021

I don't mind the padding as is, as long as it is symmetrical. The way the margins are added currently (on top of the padding), there is more space on the right than the top.

Quarto is unbalanced with more space on top than the right, which also bothers me!
Screen Shot 2021-07-12 at 3 24 23 PM

I would do .5rem all around.

@cderv
Copy link
Collaborator Author

cderv commented Jul 13, 2021

I don't think the margins are needed here anymore now (red background color added here to help visualize):

I know understand you are talking about the margin of the pre, is that it ?
I am seeing that in the screenshot looking at what rule you deactivated.

The current margin on the pre create a gap between the button and the right side of the pre block when you add the red background.
This margin on the pre is there by designed I believe to have the background of code block spread completely from left to right. (Which lead also to change in block for #1084

Currently, <button> have no margins and padding is the rule sets by bootstrap.
image

I don't mind the padding as is, as long as it is symmetrical.

What we could do is move the copy button to the right to be coherent with the pre background.

image

@cderv
Copy link
Collaborator Author

cderv commented Jul 13, 2021

If we want to reduce the red background padding, it would mean changing bootstrap default.

Possibly this would need to be done at the bootstrap variable level maybe ?

I am tweaking a lot of things at CSS level currently, but we should leverage bslib better maybe. the bs4_book.css could have been a bs4_book.scss at some point. 😄

@cderv
Copy link
Collaborator Author

cderv commented Jul 13, 2021

Updated example: https://cderv.github.io/bs4booktesting/with-scroll/final-words.html

@apreshill apreshill self-requested a review July 13, 2021 13:39
Copy link
Contributor

@apreshill apreshill left a comment

Choose a reason for hiding this comment

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

LGTM!

cderv added 2 commits July 13, 2021 16:39
[skip ci]
[skip ci]
@cderv cderv changed the title Revert change on code block regarding scrolling Revert and improve change on code block regarding scrolling Jul 13, 2021
@cderv cderv merged commit 0de8f11 into master Jul 13, 2021
@cderv cderv deleted the unscroll-code-chunk branch July 13, 2021 14:41
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants