-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Changed css classes assignment in RenderPlaceholderRow #62709
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
base: main
Are you sure you want to change the base?
Changed css classes assignment in RenderPlaceholderRow #62709
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the placeholder rendering in QuickGrid.razor
to apply a default CSS class only when no custom placeholder template is provided, and adds a new async virtualization QuickGrid scenario with corresponding E2E tests.
- In
QuickGrid.razor
, introduced logic to check forPlaceholderTemplate
and conditionally include the"grid-cell-placeholder"
class. - Added a new
VirtualizationQuickGrid.razor
test component demonstrating async loading with custom and default placeholders. - Updated
Index.razor
andVirtualizationTest.cs
to wire up and verify the new QuickGrid virtualization scenario.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/src/QuickGrid.razor | Conditionally set placeholder CSS class based on presence of custom template |
src/Components/test/testassets/BasicTestApp/VirtualizationQuickGrid.razor | New test component demonstrating async QuickGrid virtualization with placeholders |
src/Components/test/testassets/BasicTestApp/Index.razor | Added menu option for VirtualizationQuickGrid in the sample app |
src/Components/test/E2ETest/Tests/VirtualizationTest.cs | Added E2E test PlaceholdersHaveCorrectValue_Async to verify CSS classes and content |
src/Components/test/testassets/Components.TestServer/RazorComponents/ComplexValidationComponent.razor | Minor change to <p id="is-interactive"> content |
Comments suppressed due to low confidence (1)
src/Components/test/testassets/BasicTestApp/VirtualizationQuickGrid.razor:34
- The TaskCompletionSource is missing a generic type parameter, which will cause a compilation error. Specify a type argument (e.g., TaskCompletionSource<object?>) and update SetResult calls accordingly (e.g., SetResult(null)).
TaskCompletionSource asyncTcs = new TaskCompletionSource();
<td class="grid-cell-placeholder @ColumnClass(col)" @key="@col">@{ col.RenderPlaceholderContent(__builder, placeholderContext); }</td> | ||
var hasCustomPlaceholder = col.PlaceholderTemplate is not null; | ||
var placeholderClass = hasCustomPlaceholder ? "" : "grid-cell-placeholder"; | ||
<td class="@placeholderClass @ColumnClass(col)" @key="@col">@{ col.RenderPlaceholderContent(__builder, placeholderContext); }</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider trimming the combined class string to avoid leading whitespace when placeholderClass is empty, for example using class="@($"{placeholderClass} {ColumnClass(col)}".Trim())"
.
<td class="@placeholderClass @ColumnClass(col)" @key="@col">@{ col.RenderPlaceholderContent(__builder, placeholderContext); }</td> | |
<td class="@($"{placeholderClass} {ColumnClass(col)}".Trim())" @key="@col">@{ col.RenderPlaceholderContent(__builder, placeholderContext); }</td> |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This not only affects the ...
but also the opacity. (See https://github.com/dotnet/aspnetcore/blob/main/src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/src/Themes/Default.css).
I think that rather than conditionally adding/removing the class (which will make things harder for us in the future), we should keep the class and add an extra class (let's say "default" for the discussion but find a better name).
Something like
.quickgrid[theme=default] > tbody > tr > td.grid-cell-placeholder.default:after {
content: '\2026';
opacity: 0.75;
}
This more closely follows "BEM" (Block-Element-Modifier) and essentially still allows people to target the placeholder indistinctively if they want to.
Changed css classes assignment in RenderPlaceholderRow
Description
This pull request includes a change to the
QuickGrid.razor
. The update introduces logic to check for custom placeholder templates and adjusts the CSS class applied to the cell accordingly.Changes:
src/Components/QuickGrid/Microsoft.AspNetCore.Components.QuickGrid/src/QuickGrid.razor
: Added a check for whether a column has a custom placeholder template (PlaceholderTemplate
). If it does, a specific CSS class is applied; otherwise, a default "grid-cell-placeholder" class is included. This ensures better styling and flexibility for placeholder content.Fixes #49440