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

Bootstrap 5 upgrade without theme feature. #1037

Conversation

gammamatrix
Copy link
Contributor

Changes

@gammamatrix gammamatrix marked this pull request as draft May 11, 2024 23:56
@gammamatrix gammamatrix marked this pull request as ready for review May 12, 2024 00:56
@sebastianbergmann
Copy link
Owner

Are these visual changes intentional?

Before these changes (cafa435)

grafik

After these changes (ba3047f)

grafik

@gammamatrix
Copy link
Contributor Author

Are these visual changes intentional?

No. I am looking into this now. It could be browser specific.

I am reviewing the rules in style.css; the old rules definitely need a bit of modification due to a new Bootstrap 5 rule for tables:

.table>:not(caption)>*>* {
    padding: .5rem .5rem;
    color: var(--bs-table-color-state,var(--bs-table-color-type,var(--bs-table-color)));
    background-color: var(--bs-table-bg);
    border-bottom-width: var(--bs-border-width);
    box-shadow: inset 0 0 0 9999px var(--bs-table-bg-state,var(--bs-table-bg-type,var(--bs-table-accent-bg)))
}
  • This rule changes the td background-color.
  • I will test a few more browsers and operating systems to make sure the colors propagate from the table.tr to table.tr.td, as expected.
  • I have at least one minor change so far. I think I may have to alter a few more rules, once I test a bit.

@gammamatrix
Copy link
Contributor Author

Expected output

I updated a few rules, and undid a few modified rules.

  • I have not tested other browsers yet, it is possible there is an issue with the CSS function for rgb(from ...):
rgb(from var(--bs-warning) r g b / 0.1)

This is what I am seeing with the latest changes:
php-code-coverage-bootstrap-5-GH-1037

@gammamatrix
Copy link
Contributor Author

Findings

It does appear to be a browser compatibility issue with only Firefox for relative colors: https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgb#browser_compatibility

I went ahead and put back the original default colors into Colors.php:

return new self('#dff0d8', '#c3e3b5', '#99cb84', '#fcf8e3', '#f2dede');

The theme friendly rules can still be applied as environment variables:

<html outputDirectory="output/html" lowUpperBound="50" highLowerBound="90"
    colorSuccessLow="rgb(from var(--bs-purple) r g b / 0.75)"
    colorSuccessMedium="rgb(from var(--bs-blue) r g b / 0.55)"
    colorSuccessHigh="rgb(from var(--bs-teal) r g b / 0.25)"
    colorWarning="rgb(from var(--bs-warning) r g b / 0.25)"
    colorDanger="rgb(from var(--bs-danger) r g b / 0.25)" />

NOTE: Mozilla has approved relative colors, but it has not made it into Firefox yet:

@sebastianbergmann
Copy link
Owner

Thank you for working on this. Unfortunately, I found more visual differences:

Are these visual changes intentional?

Before these changes (cafa435)

grafik

After these changes (3ab35d1)

grafik

@gammamatrix
Copy link
Contributor Author

Thank you for working on this. Unfortunately, I found more visual differences:

Are these visual changes intentional?

No problem, happy to help! And no, those changes are not intentional.

In this case, the Bootstrap 5 rules are taking precedence with the use of: bg-success, bg-warning, bg-danger

.bg-success {
  --bs-bg-opacity: 1;
  background-color: rgba(var(--bs-success-rgb),var(--bs-bg-opacity)) !important;
}
  • I will take a look and see if there are some simple CSS modifications I can make to style.css.

php-code-coverage-bootstrap-progress-bars

@gammamatrix
Copy link
Contributor Author

Findings for progress-bar background colors

So it looks like the optional colors have never overridden the progress-bar. Bootstrap 4 just used a lighter color for bg-success:

.bg-success {
    background-color: #28a745 !important;
}

php-code-coverage-bootstrap-4-used-different-green

I was experimenting with some rules to allow the progress bars to override colors.

This is what it would look like with Bootstrap 4 and the override colors, which is not ideal:

php-code-coverage-bootstrap-4-override-progress-bar-colors

I have a few more things I can try, but a final solution might require the usage of themes along with CSS rgba(). If it is too complicated or messy, I will update this comment with details so we can figure out how to proceed.

@gammamatrix
Copy link
Contributor Author

gammamatrix commented May 13, 2024

Proposed changes (without data-bs-theme feature)

Sorry for so much info in this post

  • We could back out these new --phpunit-*- in this last commit, but it seems the best and cleanest way to get the progress bar colors to match the original green: --phpunit-success-bar: #28a745;
  • At the time of finishing this post, besides the failing unit test, I do not know of any other issues, in the browser or the code. Please let me know if any more are found.

In this last pass, I added root variables for --phpunit at the top of the style.css file. The variables are replaced once and defined at the top of the file:

/* :root {
 --phpunit-success-bar: #28a745;
 --phpunit-success-high: #99cb84;
 --phpunit-success-medium: #c3e3b5;
 --phpunit-success-low: #dff0d8;
 --phpunit-warning: #fcf8e3;
 --phpunit-warning-bar: #ffc107;
 --phpunit-danger: #f2dede;
 --phpunit-danger-bar: #dc3545;
} */

:root {
 --phpunit-success-bar: #28a745;
 --phpunit-success-high: {{success-high}};
 --phpunit-success-medium: {{success-medium}};
 --phpunit-success-low: {{success-low}};
 --phpunit-warning: {{warning}};
 --phpunit-warning-bar: #ffc107;
 --phpunit-danger: {{danger}};
 --phpunit-danger-bar: #dc3545;
}
  • the existing color options get set as before, with no changes to Colors.php. I added a few --phpunit-*-bar rules to cover the old Bootstrap 4 color so it looks the same.
  • So far, I have tested overriding the defined --phpunit-* in the customCssFile and that seems to be working properly in Firefox as well for the dark mode theme.

What is different in style.css

1. Lists and columns
.table tbody tr.covered-by-large-tests, .table tbody tr.covered-by-large-tests td, li.covered-by-large-tests, tr.success, tr.success td, td.success, li.success, span.success {
 background-color: var(--phpunit-success-low);
}

.table tbody tr.covered-by-medium-tests, .table tbody tr.covered-by-medium-tests td, li.covered-by-medium-tests {
 background-color: var(--phpunit-success-medium);
}

.table tbody tr.covered-by-small-tests, .table tbody tr.covered-by-small-tests td, li.covered-by-small-tests {
 background-color: var(--phpunit-success-high);
}

.table tbody tr.warning, .table tbody tr.warning td, .table tbody td.warning, li.warning, span.warning {
 background-color: var(--phpunit-warning);
}

.table tbody tr.danger, .table tbody tr.danger td, .table tbody td.danger, li.danger, span.danger {
 background-color: var(--phpunit-danger);
}
2. Covered, Not Covered and Not Coverable
.covered-by-small-tests {
 background-color: var(--phpunit-success-high);
}

.covered-by-medium-tests {
 background-color: var(--phpunit-success-medium);
}

.covered-by-large-tests {
 background-color: var(--phpunit-success-low);
}

.not-covered {
 background-color: var(--phpunit-danger);
}

.not-coverable {
 background-color: var(--phpunit-warning);
}
3. Progress Bars

This is the part the required the fix

Typically, we try to avoid using !important; however, Bootstrap 5 uses it for progress bars, so we must as well in order to customize them.

.progress-bar.bg-success {
 background-color: var(--phpunit-success-bar) !important;
}

.progress-bar.bg-warning {
 background-color: var(--phpunit-warning-bar) !important;
}

.progress-bar.bg-danger {
 background-color: var(--phpunit-danger-bar) !important;
}

How to override the rules in the customCssFile:

These rgba() calls do not use the "from" support missing in Firefox:

:root {
 --phpunit-success-bar: rgba(var(--bs-success-rgb), 1);
 --phpunit-success-high: rgba(var(--bs-success-rgb), 0.67);
 --phpunit-success-medium: rgba(var(--bs-success-rgb), 0.33);
 --phpunit-success-low: rgba(var(--bs-success-rgb), 0.1);
 --phpunit-warning: rgba(var(--bs-warning-rgb), 0.1);
 --phpunit-warning-bar: rgba(var(--bs-warning-rgb), 1);
 --phpunit-danger: rgba(var(--bs-danger-rgb), 0.1);
 --phpunit-danger-bar: rgba(var(--bs-danger-rgb), 1);
}
  • The screenshots in this post are using these rules in a file defined in customCssFile to show that it is easy to override and use the new CSS variables.
  • These rules reflect the Bootstrap 5 colors and work with light, dark and custom themes: Adds theme support to code coverage. phpunit#5833

How it looks in the browser

This section shows the way the UI works with the commit:

  • Bootstrap 5: implemented custom CSS variables: --phpunit-* 05f9d99

The examples showing dark mode were manually edited in the browser:

<html lang="en" data-bs-theme="dark">

Expand the four screenshots:

1. Firefox with CSS Vars

This is the latest changes in style.css. A few more CSS rules were added it the end to make the progress bars match.

phpunit-css-vars-firefox

2. Firefox with Dark Theme enabled

This is here just to show that the variables need to be overridden to support themes.

phpunit-css-vars-firefox-dark-theme-original-colors

3. Firefox with Light Theme enabled

Using rules shown above in customCssFile.

phpunit-css-vars-firefox-light-theme-updated-colors

4. Firefox with Dark Theme enabled

Using rules shown above in customCssFile.

phpunit-css-vars-firefox-dark-theme-updated-colors

Tests

  • NOTE: There is a unit test that fails saying the files do not match:
There was 1 failure:

SebastianBergmann\CodeCoverage\Report\Html\EndToEndTest::testPathCoverageForBankAccountTest
BankAccount.php_path.html not match
Failed asserting that string matches format description.
--- Expected
+++ Actual

/Users/jeremy/srv/sites/site-playground-make/vendor/gammamatrix/playground-make-controller/vendor/phpunit/php-code-coverage/tests/tests/Report/Html/EndToEndTest.php:108
/Users/jeremy/srv/sites/site-playground-make/vendor/gammamatrix/playground-make-controller/vendor/phpunit/php-code-coverage/tests/tests/Report/Html/EndToEndTest.php:56

@sebastianbergmann
Copy link
Owner

I just tested with the current state of the update-to-bootstrap-5-without-theme-feature branch (at ea94640) and I still see the large amount of whitespace on each line between the line number and the code that I posted a screenshot of in #1037 (comment).

Sorry for not getting back to this sooner.

@gammamatrix
Copy link
Contributor Author

I just tested with the current state of the update-to-bootstrap-5-without-theme-feature branch (at ea94640) and I still see the large amount of whitespace on each line between the line number and the code that I posted a screenshot of in #1037 (comment).

Sorry for not getting back to this sooner.

No worries, I will update my local branches and have a look at the tests to see why the extra whitespace is appearing in the test.

@sebastianbergmann
Copy link
Owner

My bad: I just noticed that the large amount of whitespace between the left "edge" and the line number also exists in the current releases and therefore cannot be caused by this PR.

@sebastianbergmann
Copy link
Owner

Here is what I currently see:

main (at e73593b)

Directory

grafik

File

grafik

update-to-bootstrap-5-without-theme-feature (at ea94640)

Directory

grafik

File

grafik

Can we restore the styling of the top navigation / breadcrumbs and reduce the whitespace between the line numbers and the beginning of the lines? Thanks!

@gammamatrix
Copy link
Contributor Author

gammamatrix commented Aug 10, 2024

Breadcrumbs Changes

  • I added another css variable for the breadcrumbs:
:root {
 --phpunit-breadcrumbs: #e9ecef;

These style changes are visible in the inspector on the screenshot:

nav .breadcrumb {
  border-radius: var(--bs-border-radius);
  background-color: var(--phpunit-breadcrumbs);
  padding: .75rem 1rem;
}
  • From Chrome, looked similar on Firefox.

php-code-coverage-GH-1037-breadcrumbs-restored-ff

  • Firefox

php-code-coverage-GH-1037-breadcrumbs-restored-chrome

  • Chrome

Line number changes

The line number column had a text-right CSS class that I switched to text-end.

CSS Variables

The template variables in style.css

:root {
 --phpunit-breadcrumbs: var(--bs-gray-200);
 --phpunit-success-bar: #28a745;
 --phpunit-success-high: {{success-high}};
 --phpunit-success-medium: {{success-medium}};
 --phpunit-success-low: {{success-low}};
 --phpunit-warning: {{warning}};
 --phpunit-warning-bar: #ffc107;
 --phpunit-danger: {{danger}};
 --phpunit-danger-bar: #dc3545;
}

Bootstrap 4 was using Gray 200. I updated the breadcrumbs to use the variable.

php-code-coverage-GH-1037-breadcrumbs-phpunit-css-vars

gammamatrix added a commit to gammamatrix/php-code-coverage that referenced this pull request Aug 10, 2024
@sebastianbergmann sebastianbergmann force-pushed the update-to-bootstrap-5-without-theme-feature branch from 17c2563 to 78c5bab Compare September 5, 2024 14:39
@sebastianbergmann sebastianbergmann merged commit 78c5bab into sebastianbergmann:main Sep 5, 2024
2 of 14 checks passed
@sebastianbergmann
Copy link
Owner

Sorry for bothering you with this, but after merging this I see the whitespace left of the line numbers again:

grafik

Also, I think the line numbers were not underlined in the past.

@gammamatrix
Copy link
Contributor Author

Sorry for bothering you with this, but after merging this I see the whitespace left of the line numbers again:

grafik

Also, I think the line numbers were not underlined in the past.

I will have a look.

@gammamatrix
Copy link
Contributor Author

Dev Notes

I was able to get a rule that works:

table#code {
  td:nth-child(1) {
    padding-left: .75em;
    padding-right: .75em;
    a {
      text-decoration: none;
     }
  }
}
  • Bootstrap 5 no longer adds text-decoration: none; to links.

GH-1037-css-fix-for-first-column

I will open up a new PR instead of using this branch.

gammamatrix added a commit to gammamatrix/php-code-coverage that referenced this pull request Sep 5, 2024
@gammamatrix
Copy link
Contributor Author

gammamatrix added a commit to gammamatrix/php-code-coverage that referenced this pull request Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants