Core: Change favicon based on testing module status#31756
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 files reviewed, 3 comments
Edit PR Review Bot Settings | Greptile
| if (!hasTestProviders && (!errorCount || !warningCount)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
logic: Condition (!errorCount || !warningCount) uses OR operator which means the module returns null if either count is 0. Should likely be AND operator (!errorCount && !warningCount) to only hide when both are 0
| if (status.value === 'status-value:success') { | ||
| result.successCount += 1; | ||
| } |
There was a problem hiding this comment.
style: Consider moving status value strings to constants to avoid repetition and ensure consistency
|
View your CI Pipeline Execution ↗ for commit d154429.
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 31.89 MB | 31.90 MB | 🚨 +11 KB 🚨 |
| Dependency size | 17.41 MB | 17.41 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 199 | 209 | 🚨 +10 🚨 |
| Self size | 189 KB | 607 KB | 🚨 +418 KB 🚨 |
| Dependency size | 29.70 MB | 29.79 MB | 🚨 +94 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
sb
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1 KB | 1 KB | 0 B |
| Dependency size | 49.29 MB | 49.30 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 216 | 324 | 🚨 +108 🚨 |
| Self size | 582 KB | 235 KB | 🎉 -347 KB 🎉 |
| Dependency size | 94.67 MB | 99.90 MB | 🚨 +5.23 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 267 | 🚨 +82 🚨 |
| Self size | 31 KB | 31 KB | 🚨 +361 B 🚨 |
| Dependency size | 78.74 MB | 82.79 MB | 🚨 +4.05 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
ndelangen
left a comment
ndelangen
left a comment
There was a problem hiding this comment.
I like this feature, but I think it should be implemented differently.
We should overlay the icons over the favicon instead of have multiple favicon files.
That way we can continue to support this cool feature when the user defines their own favicon, and not burden them with creating a whole set. Additionally, if would allow us to add states and icons at a later stage, without impacting users.
We should maintain, and support users defining.. a single favicon.
The visual effect can be maintained and achieved using svg masking:
https://developer.mozilla.org/en-US/docs/Web/SVG/Reference/Element/mask
https://webdesign.tutsplus.com/a-comprehensive-guide-to-clipping-and-masking-in-svg--cms-30380t
Though I'm unsure if that SVG-feature is fully supported in the favicon rendering in all browsers, we should ensure a sane fallback. I suspect that all chromium based browsers will just be able to do it.
| initialIcon ??= link.href || `./favicon.svg`; | ||
|
|
||
| const href = initialIcon.replace(/([^/]+)(\.\w+)([?#].*)?$/, `$1${suffix}$2$3`); | ||
| const img = new Image(); | ||
| img.onload = () => (link.href = href); | ||
| img.onerror = () => (link.href = initialIcon!); | ||
| img.src = href; |
There was a problem hiding this comment.
We could use something like this here:
<link rel="icon" type="image/svg+xml" href="data:image/svg+xml,[YOUR ENCODED SVG HERE]" />Allowing you to do the masking of an image, and render the icon on top, inline.
@ndelangen browsers don't support overlaying icons on top of each other, you can't just define multiple favicons (ie. the base and a notification dot) and then the browser will combine them together. The browser will just take the first one it finds. So for that to work we'd have to combine the SVG-code of the base icon (Storybook icon or the user's custom one) with the status dot, and that sounds very complicated. And if we attempted this, it would only work for base favicons that were SVGs, not .ico or .png. It doesn't sound like the right ROI to me, I feel like it's okay to say that this feature only works with the default favicon, and gets disabled if a user sets their own favicon. |
| faviconPaths.forEach((faviconPath) => { | ||
| const faviconDir = resolve(faviconPath, '..'); | ||
| const faviconFile = basename(faviconPath); | ||
| app.use('/', (req, res, next) => { | ||
| if (req.url === `/${faviconFile}`) { | ||
| return sirvWorkaround(faviconDir, { | ||
| dev: true, | ||
| etag: true, | ||
| extensions: [], | ||
| })(req, res, next); | ||
| } | ||
| next(); | ||
| }); |
There was a problem hiding this comment.
maybe it would be better if we could do this loop within the middleware. So instead of creating 6+ middlewares (app.use() calls), we'd still only have one, and then the loop within it would return the correct thing.
Maybe:
| faviconPaths.forEach((faviconPath) => { | |
| const faviconDir = resolve(faviconPath, '..'); | |
| const faviconFile = basename(faviconPath); | |
| app.use('/', (req, res, next) => { | |
| if (req.url === `/${faviconFile}`) { | |
| return sirvWorkaround(faviconDir, { | |
| dev: true, | |
| etag: true, | |
| extensions: [], | |
| })(req, res, next); | |
| } | |
| next(); | |
| }); | |
| app.use('/', (req, res, next) => { | |
| for (const faviconPath of faviconPaths) { | |
| const faviconDir = resolve(faviconPath, '..'); | |
| const faviconFile = basename(faviconPath); | |
| if (req.url === `/${faviconFile}`) { | |
| return sirvWorkaround(faviconDir, { | |
| dev: true, | |
| etag: true, | |
| extensions: [], | |
| })(req, res, next); | |
| } | |
| } | |
| next(); | |
| }); |
| const setFavicon = (link: HTMLLinkElement, suffix: string = '') => { | ||
| initialIcon ??= link.href || `./favicon.svg`; | ||
|
|
||
| const href = initialIcon.replace(/([^/]+)(\.\w+)([?#].*)?$/, `$1${suffix}$2$3`); |
There was a problem hiding this comment.
we need a comment explaining what this regex does.
| const img = new Image(); | ||
| img.onload = () => (link.href = href); | ||
| img.onerror = () => (link.href = initialIcon!); | ||
| img.src = href; |
There was a problem hiding this comment.
is this essentially "try to load the favicon file with the correct suffix, if it fails then revert back"?
sounds like it will produce a lot of annoying 404 message in the Network and Console DevTools, I think that should be avoided, we should find another way.
Also, won't this cause the user's favicon to be replaced with our own with a status dot if the user has called their favicon favicon.svg?
|
@JReinhold I think all your points/comments would be solved by my suggestion to add the icon to the favicon dynamically |
|
@ndelangen I reimplemented this in a different way: #31763 |
3 participants
What I did
Implemented a
useFaviconhook in the testing module to change the favicon based on its status.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
Implements dynamic favicon updates based on test execution status in Storybook's TestingModule.
useFaviconhook incode/core/src/manager/components/sidebar/useFavicon.tsto manage favicon state changescode/core/src/core-server/presets/common-preset.tsto return multiple favicon paths instead of single pathTestingModulecomponent to reflect test status through favicon (crashed > errors > warnings > running > success)SidebarBottomcomponent for accurate status representation