Uh oh!
There was an error while loading. Please reload this page.
Clear the modernization sequence's tidy-ups - #2444
Merged
Merged
Conversation
Three unrelated leftovers from #2391: - Button Swap's browser test was named 'Swap UI state when clicked' but never clicks -- this package ships the markup, not the swap script. Name it for what it asserts, and assert the half the name claimed but did not cover: that the swapped state is present and hidden. - Root-level TypeScript belonged to no tsconfig project, so the type-aware lint rules could not see it. #2421 worked around that with a hand-maintained allowDefaultProject list, which is what let test-utils.ts go unlinted. Add root-level globs to tsconfig instead, so the next file is covered automatically and gets our real strictness rather than an inferred project's defaults. This pulled twing/vite-plugin-twig.mjs into the program under checkJs, hence the annotations there. - src/index-with-dependencies.scss was the last @import in the repo. Dart Sass 3 removes @import. Fixes#2431
🦋 Changeset detectedLatest commit: d651812 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for cloudfour-patterns ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Three unrelated leftovers from the #2391 modernization sequence, which would otherwise have vanished when that issue closed.
Button Swap's test name. It was called
Swap UI state when clickedand never clicked — this package ships the component's markup but not the script that swaps it, so there is nothing to click. Rather than just renaming it, the test now also asserts the half the old name implied but never checked: that the swapped state is in the markup and ishidden. The aria snapshot already covered the other half, sincehiddenkeeps that subtree out of the accessibility tree.Root-level TypeScript and the type-aware lint rules.
tsconfig.jsoncovered onlysrc, so root-level tooling files belonged to no project. #2421 worked around that with a hand-maintainedallowDefaultProjectlist, which is the mechanism that lettest-utils.tssit unlinted from #2416 until #2421 deleted it. Of the two options in the issue, extending the config came out cleaner — there is nowhere sensible to relocatevitest.config.mtsto that a project already covers, since the only covered directory is the one that ships. Root-level globs inincludemean the next file is picked up without anyone remembering, and those files now get our actualstrictsettings instead of an inferred project's defaults. That last part is whytwing/vite-plugin-twig.mjsgains four JSDoc annotations: addingvitest.config.mtspulls the plugin it imports into the program, wherecheckJsfinally looks at it.@useinstead of@import.src/index-with-dependencies.scssheld the last two@imports in the repo, and Dart Sass 3 removes the rule. See the note below on what this did to the compiled output — the change is not output-neutral, and it is worth a look before approving.Screenshots
Testing
@font-facerule went missing.One thing to look at before approving
The
@useswap is not output-neutral, so per the bail-out condition in #2431 here is exactly what changed.Compiling the file directly, and in the real Storybook build, all non-
@font-faceCSS is byte-for-byte identical. The only difference is that seven@font-facerules used to be emitted twice and are now emitted once. The surviving seven are byte-for-byte the originals, in the original order — the built Storybook stylesheet goes from 14 blocks to 7 and shrinks by 1,245 bytes.The cause is the load-once semantics the issue flagged, landing the favourable way round:
./indexloadsbase/fontsas well, and an@importdoes not share the module graph with the file importing it, so the fonts were emitted a second time.@useloads each stylesheet once.I read this as a pass rather than the bail-out, because nothing is dropped, reordered, or newly duplicated, and a browser handed the same
@font-faceset twice behaves identically to being handed it once. But it is a judgment call against the issue's wording, so if you would rather have byte-identical output, dropping this one file's changes leaves the other two items intact.To reproduce:
Why this has a changeset
#2440 lists this issue as needing none, on the strength of the issue body saying the Sass file is excluded from the published
files. It is not —filesexcludessrc/index.scssbut notsrc/index-with-dependencies.scss, andnpm pack --dry-runconfirms the file ships. So this changes published output and takes a patch.Worth a separate issue: that file is documented as Storybook-only and hard-codes
/src/assets/fonts, a root-absolute path that cannot mean anything to a consumer. It probably should not be in the tarball at all, which is the same "what is actually our public surface" question as #2078.