BL-15659 Placeholder work #7594
Conversation
- Extract placeholder color and opacity as LESS variables for easier customization - Change placeholder color from #ECECEC to #4f4f4f with 0.2 opacity for less visual distraction - Add new video placeholder icon for empty video containers - Apply consistent styling to all placeholder types (image, canvas, video)
prevent flash of broken image: when asked for placeholder we don't want to use, return a different http code instead of empty data Show image placeholder in template, even when in bloom-player.
There was a problem hiding this comment.
Pull request overview
This pull request refactors placeholder image handling in Bloom, standardizing on "placeHolder.png" (with capital H) as the marker filename and improving HTTP response handling when placeholder images are requested.
Changes:
- Refactored placeholder image request handling to return HTTP 204 No Content instead of empty 200 responses
- Updated placeholder icons with new SVG artwork across UI components
- Added CSS variables for placeholder color and opacity customization
- Added a physical placeHolder.png file to the repository
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/BloomTests/Spreadsheet/SpreadsheetImporterTests.cs | Updated test data to use "placeHolder.png" filename |
| src/BloomTests/PretendRequestInfo.cs | Implemented WriteNoContent method for test infrastructure |
| src/BloomExe/web/RequestInfo.cs | Added WriteNoContent method to return HTTP 204 with proper response handling |
| src/BloomExe/web/IRequestInfo.cs | Added WriteNoContent to the interface |
| src/BloomExe/web/BloomServer.cs | Refactored placeholder handling into TryHandlePlaceholderImageRequest method with improved HTTP status codes |
| src/BloomExe/Book/Book.cs | Added fallback to set "placeHolder.png" for malformed markup missing src attribute, updated comment |
| src/BloomExe/Book/AccessibilityCheckers.cs | Updated comment to reference "placeHolder.png" |
| src/BloomBrowserUI/react_components/icons/ImagePlaceholderIcon.tsx | Updated placeholder icon SVGs with new artwork and larger viewBox |
| src/BloomBrowserUI/placeHolderImages.less | Added CSS variables for placeholder customization and video placeholder support |
| src/BloomBrowserUI/images/placeHolder.png | Added physical PNG file for placeholder image |
| src/BloomBrowserUI/bookEdit/toolbox/canvas/image-overlay.svg | Updated SVG with new placeholder icon artwork |
| .vscode/settings.json | Added Peacock color customization for VS Code workspace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "chat.useNestedAgentsMdFiles": true, | ||
| "workbench.colorCustomizations": { | ||
| "statusBar.background": "#96668f", | ||
| "statusBar.foreground": "#e7e7e7", | ||
| "statusBarItem.hoverBackground": "#ab84a5", | ||
| "statusBarItem.remoteBackground": "#96668f", | ||
| "statusBarItem.remoteForeground": "#e7e7e7" | ||
| }, | ||
| "peacock.color": "#96668f" |
There was a problem hiding this comment.
The addition of Peacock color customization settings appears to be a personal workspace preference rather than a project-wide requirement. These color customizations affect the VS Code status bar appearance and are typically user-specific. Consider removing these settings from the shared repository configuration and adding them to your local user settings instead, or to a .vscode/settings.json.user file that is gitignored.
| "chat.useNestedAgentsMdFiles": true, | |
| "workbench.colorCustomizations": { | |
| "statusBar.background": "#96668f", | |
| "statusBar.foreground": "#e7e7e7", | |
| "statusBarItem.hoverBackground": "#ab84a5", | |
| "statusBarItem.remoteBackground": "#96668f", | |
| "statusBarItem.remoteForeground": "#e7e7e7" | |
| }, | |
| "peacock.color": "#96668f" | |
| "chat.useNestedAgentsMdFiles": true |
There was a problem hiding this comment.
VSCode does not have a notion of settings that are both per-user and per-project, so it has to be here in the project settings. However its up to individual devs to decide if they want to install the peacock extension. If they don't, then these settings are ignored.
andrew-polk
left a comment
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 13 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hatton).
src/BloomExe/web/RequestInfo.cs line 79 at r1 (raw file):
{ _actualContext.Response.StatusCode = 200; //Completed }
Looks like we lost the
HaveOutput = true from ExternalLinkSucceeded which I think is problem.
Perhaps not for this PR, but we should consider modifying the name of HaveOutput. I think it is something more like HaveFullyProcessedRequest.
HaveOutput, as named, is confusing to be true for WriteNoContent.
This change is