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

CSS: Add a debugger class. #40976

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

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Oct 24, 2024

Warning

Remove 186bda9 before merge.
Feel free to add the dist files if needed

Description

Add an option to help people use the grid by adding a debugger inside their website.
Basically, it's only a .bs-grid-debug class that you add on any element to see all the children grids initialized on all .container-*s and .rows (doesn't take into account the paddings added manually).

I've added it as opt-in in the library, in this way:

  • Opt-in for people who need it, so only one parameter is needed to be checked to have it for all Sass users.
  • A new bundle so people (that will probably the most use it) can use it from CDN directly.

There is many ways to implement it, so feel free to debate or argue in a way or another. For example, there are surely many other ways, but I can think of at least:

  • Offer it by default for bootstrap-grid.css and bootstrap.css since it's not much now and can bring much for newcomers. I think I'd add it like this if it were only me.
  • Offer it only as opt-in for advanced Sass users only.
  • The actual implementation that is a different package + opt-in.
  • Add it by default (but cancel the fact that we can choose which grid to show)
  • Limit the propagation of the grid show only one layer (the one selected)

Depending on the answers below, I'll adapt the content if needed.

Feel free to debate with me the design choices.

Motivation & Context

Help people debug their grids.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Please add .debug on any element inside any page to see it live.

Related issues

Closes #40741.
Closes #40927.

@@ -0,0 +1,3 @@
$enable-debugger-classes: true; // stylelint-disable-line scss/dollar-variable-default

@import "bootstrap";
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would include only scss/_debug.scss (and the right imports for the compilation), so that bootstrap-debugger.css contains just the debugging elements. And then, no need for an RTL version.

And the other scss/bootstrap-*.scss wouldn't include @import "debug".

That way, everybody is using the same bootstrap*.css files (with the same sha384) and could enable the debugging part via Sass compilation and $enable-debugger-classes, or via CDN by importing bootstrap-debugger.css after (I suppose) the regular Bootstrap CSS files (RTL or not).

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand your point, I'va made the changes but for me, we should either have a $enable-debugger-classes and use @import in the good files or have no option and so let the users decide when they import it or not.

scss/_debug.scss Outdated
@@ -0,0 +1,44 @@
@if $enable-debugger-classes {
.debug {
Copy link

Choose a reason for hiding this comment

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

more specific naming like .bs-debug or .grid-breakpoint-debug will be good to avoid conflicts of user space class names.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes definitely, i'm changing it.

}
}

[class^="container"],
Copy link

Choose a reason for hiding this comment

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

Can you provide this functionality as a seperated class name?

Copy link
Member Author

@louismaximepiton louismaximepiton Nov 13, 2024

Choose a reason for hiding this comment

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

Why not, I don't have strong opinion on this. I did it this way because on my project, we use sometimes .row and sometimes .container-* with .col-* inside. Do you have any cool name to propose ?

Copy link

Choose a reason for hiding this comment

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

This feature looks nice but the developer may want to toggle on/off this option to make the design look more clear and understandable.

You know, design considerations call for pixel-perfect work, and the overlay of this option can become annoying at some point.

.bs-debug-grid-marks, .bs-debug-grid-cols or something else like to understand of the purpose this utility.

It would be better if it does not have any connection with the other class name. The developer can use whatever he/she wants like <body class="bs-debug-grid-breakpoints bs-debug-grid-marks">

@@ -383,6 +383,7 @@ $enable-validation-icons: true !default;
$enable-negative-margins: false !default;
$enable-deprecation-messages: true !default;
$enable-important-utilities: true !default;
$enable-debugger-classes: true !default;
Copy link

Choose a reason for hiding this comment

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

default value looks true

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, it's done on purpose at the moment to be able to test it on the deploy preview but I think it will go to false before getting merged.

color: $black;
background-color: $white;
border-radius: 0 0 4px; // stylelint-disable-line property-disallowed-list
@include font-size($h1-font-size);
Copy link

Choose a reason for hiding this comment

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

can this get font size from css a variable? then we may able to adjust it like <body style="--bs-debug-label-font-size: 15px;">

top: 0;
left: 0;
z-index: 2000;
padding: $spacer * .5 $spacer;
Copy link

Choose a reason for hiding this comment

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

can this use em unit instead of rem unit for padding? Then it may adjust itself by its font size.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

Breakpoint indicator label Introduce Grid Debugger Class for Bootstrap Containers
3 participants