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

Fix inconsistent behavior of code-folding buttons #2085

Merged
merged 6 commits into from
Apr 20, 2021
Merged

Fix inconsistent behavior of code-folding buttons #2085

merged 6 commits into from
Apr 20, 2021

Conversation

steveharoz
Copy link
Contributor

@steveharoz steveharoz commented Mar 30, 2021

✔ When a block of code is initialized as hide, that block's show/hide button updates when pressed. The button's aria-expanded attribute is set to true/false, and it adds/removes the class collapsed as expected.

❌ But when a block of code is initialized as show, that block's show/hide button is locked with aria-expanded="true" and no collapsed class.

This issue has accessibility implications and limits customizing the css of the button.

To reproduce

  1. Grab an RMarkdown file of choice with code blocks.
  2. Add a CSS file (css: style.css) with the code button.code-folding-btn[aria-expanded = "false"] {background-color: blue;}
  3. Add code-folding:show and knit as HTML.
  4. Notice that pressing a block's show/hide button does NOT update the DOM
  5. Change to code-folding:hide, and it updates.

What the fix does

Expanded code blocks are currently shown before the button is created. This fix just defers until the button is already added.


Session info

> xfun::session_info('rmarkdown')
R version 4.0.3 (2020-10-10)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 19041), RStudio 1.4.1103

Locale:
  LC_COLLATE=English_United States.1252  LC_CTYPE=English_United States.1252    LC_MONETARY=English_United States.1252 LC_NUMERIC=C                          
  LC_TIME=English_United States.1252    

Package version:
  base64enc_0.1.3   digest_0.6.27     evaluate_0.14     glue_1.4.2        graphics_4.0.3    grDevices_4.0.3   highr_0.8         htmltools_0.5.1.1 jsonlite_1.7.2   
  knitr_1.31        magrittr_2.0.1    markdown_1.1      methods_4.0.3     mime_0.10         rlang_0.4.10      rmarkdown_2.7.4   stats_4.0.3       stringi_1.5.3    
  stringr_1.4.0     tinytex_0.30      tools_4.0.3       utils_4.0.3       xfun_0.21         yaml_2.2.1       

Pandoc version: 2.11.2

@CLAassistant
Copy link

CLAassistant commented Mar 30, 2021

CLA assistant check
All committers have signed the CLA.

The events were firing when hiding finished and showing started. This update makes all show/hide started/finished combinations visible to CSS.
Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

I didn't test it but it sounds about right to me. I'll let @cderv review it later. Thanks!

@yihui yihui requested a review from cderv April 18, 2021 03:08
Copy link
Collaborator

@cderv cderv left a comment

Choose a reason for hiding this comment

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

It looks good to me too. I tested it and it works ok now. Thanks for catching this.

One question though: Why add some temporary classes between hide and hidden (code-collapsing) and show and shown (code-expanding)?

@steveharoz
Copy link
Contributor Author

steveharoz commented Apr 19, 2021

@cderv

Why add some temporary classes between hide and hidden (code-collapsing) and show and shown (code-expanding)?

Bootstrap adds an intermediate class (collapsing) to the expanding div (the code) for intermediate styles. This PR adds something to the button too for consistency. I couldn't use collapsing because bootstrap already uses it, so I used code-collapsing.

You could get away only defining the code-collapsing class and combining it with the aria-expanded state to determine the direction. But I thought it'd be easier to differentiate direction in the class name with code-expanding.

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

Oh so this is useful for styling during the transition ? I never done it myself.
What would be the issue to use the same class as bootstrap is using on the div for this button we created ?

You could target div.r-code-collapse.collapsing for the divs or button.code-folding-btn.collapsing for the button if you need to customize the transition. right ?

I am just trying to understand completely as I want to avoid adding a specific class in rmarkdown if it is not necessary.

@steveharoz
Copy link
Contributor Author

What would be the issue to use the same class as bootstrap is using on the div for this button we created ?

Good question. That's what I tried first. Unfortunately, bootstrap already has styles that apply to .collapsing, especially the CSS animations. So if the button used that same class name, the button would change size and have weird issues during the transition. And we'd need to find a way to override that.

Bootstrap's .collapsing definition: https://github.com/twbs/bootstrap/blob/f61a0218b36d915db80dc23635a9078e98e2e3e0/scss/_transitions.scss

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

Oh that makes complete sense ! Then new class it is !

What about btn-collapsing as class name so that it is clearer that this target the button ?
I am fine with code-collapsing, it just has puzzled me first when I saw this and I did not get it right that it applies to button and not the code part.

@steveharoz
Copy link
Contributor Author

Sure. I have no attachement to the class name. I can update it to btn-collapsing and btn-expanding later today.

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

I'll do it and merge.

I wonder where we should document that. I'll put it in NEWS only for now. Thanks a lot @steveharoz !

@steveharoz
Copy link
Contributor Author

Cool. Cheers!

@cderv cderv merged commit 6e572ca into rstudio:master Apr 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 19, 2021
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.

None yet

4 participants