insightsengineering / teal.reporter

Create and preview reports with Shiny modules
https://insightsengineering.github.io/teal.reporter/
Other
8 stars 8 forks source link

187 color scheme for disabled button #252

Closed chlebowa closed 4 months ago

chlebowa commented 4 months ago

Closes #187

Updated background color of two buttons (anchor elements) identified by partial ids in disabled state.

The background color is darkgrey nad text color is white.

image image

github-actions[bot] commented 4 months ago

badge

Code Coverage Summary

Filename              Stmts    Miss  Cover    Missing
------------------  -------  ------  -------  -----------------------------------------------------------------------------------
R/AddCardModule.R       144       2  98.61%   165, 202
R/Archiver.R             25       0  100.00%
R/ContentBlock.R         18       2  88.89%   57-63
R/DownloadModule.R      207      49  76.33%   98-104, 146, 177-182, 191-195, 198-202, 210-214, 217-221, 228-232, 235-239, 278-282
R/FileBlock.R            13       0  100.00%
R/NewpageBlock.R          2       0  100.00%
R/PictureBlock.R         30       2  93.33%   20, 118
R/Previewer.R           295      56  81.02%   198, 213, 215-218, 221, 224-232, 346-390
R/RcodeBlock.R           15       0  100.00%
R/Renderer.R            113      37  67.26%   97-112, 216, 224, 233, 235-256
R/ReportCard.R           77       4  94.81%   192, 233, 238, 259
R/Reporter.R             94       1  98.94%   270
R/ResetModule.R          55       0  100.00%
R/SimpleReporter.R       30       0  100.00%
R/TableBlock.R            9       0  100.00%
R/TextBlock.R            13       0  100.00%
R/utils.R               171      80  53.22%   7, 38-97, 99, 102-109, 167, 179-181, 285-294
R/yaml_utils.R           81       2  97.53%   79, 290
R/zzz.R                  14      10  28.57%   2-13, 19
TOTAL                  1406     245  82.57%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: 7fcd93b5816282a2d121f0be8d2dd281698cbb3d

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

github-actions[bot] commented 4 months ago

Unit Tests Summary

  1 files   18 suites   12s :stopwatch: 204 tests 204 :white_check_mark: 0 :zzz: 0 :x: 346 runs  346 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 7fcd93b5.

:recycle: This comment has been updated with latest results.

Polkas commented 4 months ago

Please be careful and check it out on all 3 bootstrap versions. The issue is connected only with bootstrap version 3. However, the code update will influence all of them (3, 4 and 5). A class rule seems more natural than an id suffix-based selector.

Polkas commented 4 months ago

the download buttons are a tags in Shiny, and the solution is probably to edit the already existing rule from the same file.

/* Disable any anchor with disabled class */
a.disabled {
  color: grey;
  cursor: not-allowed;
  pointer-events: none;
  text-decoration: none;
}

color: lightgrey; (or white) seems to fix the bootstrap 3 and is omitted for versions 3 and 4. Even if not omitted for versions 4 and 5, it still looks ok.

chlebowa commented 4 months ago

check it out on all 3 bootstrap versions

Good point :+1:

A class rule seems more natural than an id suffix-based selector.

This was intentional. I didn't want to affect other a elements because, frankly, I wasn't sure how many others there are. This solution affects these two download buttons only.

Polkas commented 4 months ago

The a.disabled rule is already in the css file and narrows the scope only to disabled a tags. So please consider my proposition.

chlebowa commented 4 months ago

Another good point :slightly_smiling_face:

chlebowa commented 4 months ago

Well, styling by class only seems to work with BS 3 :shrug: Styling by id works in BS 3, 4, and 5.

Polkas commented 4 months ago

I assumed we wanted to fix Bootstrap 3 only, and omitting a rule for versions 4 and 5 seemed ok to me. If we want to apply it to all of them, we must use an !important solution.

/* Disable any anchor with disabled class */
a.disabled {
  color: white !important;
  cursor: not-allowed;
  pointer-events: none;
  text-decoration: none;
}
chlebowa commented 4 months ago

I am wary of using !important on a tag. There is a risk of the style spilling out to other elements (a tags in other modules). I think it is safer to narrow down the specification by id.

How about a[id$=download_data].disabled?

Polkas commented 4 months ago

Looks nice now:) and border:0 improve it. An edge case are bootswatch themes (bslib::bootswatch_themes()), but I checked out a few and the update is okay for them. Bootstrap 5

Screenshot 2024-02-22 at 17 14 19

Bootstrap 4

Screenshot 2024-02-22 at 17 13 33

Bootstrap 3

Screenshot 2024-02-22 at 17 11 35
chlebowa commented 4 months ago

Awesome, thanks for checking :+1: :slightly_smiling_face: