feat: add option to change html lang#34501
Open
mehm8128 wants to merge 1 commit intostorybookjs:nextfrom
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe changes add support for a configurable HTML language attribute ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Config
participant Presets
participant BuilderManager
participant Builder (Vite/Webpack)
participant Template
User->>Config: set htmlLang (optional)
Config->>Presets: apply('htmlLang')
Presets-->>BuilderManager: htmlLang value
BuilderManager->>Builder: renderHTML(..., htmlLang)
Builder->>Template: provide templateParameters (htmlLang)
Template-->>User: rendered HTML with lang=(htmlLang || 'en')
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/builders/builder-webpack5/templates/preview.ejs (1)
2-2: Consider adding a defensive fallback for consistency.The server template (
code/core/assets/server/template.ejs) uses a defensive check:typeof htmlLang !== 'undefined' ? htmlLang : 'en'. This template directly uses<%= htmlLang %>without a fallback.While the preset system should always provide a default value of
'en', adding a consistent fallback would be more defensive:<html lang="<%= typeof htmlLang !== 'undefined' ? htmlLang : 'en' %>">This is a minor inconsistency and the current implementation should work correctly given the preset guarantees.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-webpack5/templates/preview.ejs` at line 2, The template uses <%= htmlLang %> directly which lacks a defensive fallback; update the preview.ejs HTML tag to use a typeof check on the htmlLang variable (same pattern as server/template.ejs) so it falls back to 'en' when htmlLang is undefined — locate the <html lang="..."> line in preview.ejs and replace the direct <%= htmlLang %> usage with a ternary typeof htmlLang !== 'undefined' ? htmlLang : 'en' expression.code/core/src/builder-manager/utils/template.ts (1)
48-48: NormalizehtmlLangbefore fallback to avoid invalid whitespace values.
(await htmlLang) || 'en'won’t catch whitespace-only inputs, so<html lang=" ">can slip through.Proposed refactor
- htmlLang: (await htmlLang) || 'en', + htmlLang: (await htmlLang)?.trim() || 'en',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/builder-manager/utils/template.ts` at line 48, The current assignment htmlLang: (await htmlLang) || 'en' allows whitespace-only values to pass through; update the logic around htmlLang (the awaited value used for htmlLang) to trim and check emptiness before falling back to 'en' — i.e., await htmlLang into a temporary (or apply .then/.trim) then if the trimmed string is empty use 'en'; locate the usage of htmlLang in template.ts and replace the direct fallback with a normalized-trim-and-empty-check to ensure values like " " do not become the HTML lang attribute.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/builders/builder-webpack5/templates/preview.ejs`:
- Line 2: The template uses <%= htmlLang %> directly which lacks a defensive
fallback; update the preview.ejs HTML tag to use a typeof check on the htmlLang
variable (same pattern as server/template.ejs) so it falls back to 'en' when
htmlLang is undefined — locate the <html lang="..."> line in preview.ejs and
replace the direct <%= htmlLang %> usage with a ternary typeof htmlLang !==
'undefined' ? htmlLang : 'en' expression.
In `@code/core/src/builder-manager/utils/template.ts`:
- Line 48: The current assignment htmlLang: (await htmlLang) || 'en' allows
whitespace-only values to pass through; update the logic around htmlLang (the
awaited value used for htmlLang) to trim and check emptiness before falling back
to 'en' — i.e., await htmlLang into a temporary (or apply .then/.trim) then if
the trimmed string is empty use 'en'; locate the usage of htmlLang in
template.ts and replace the direct fallback with a
normalized-trim-and-empty-check to ensure values like " " do not become the
HTML lang attribute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86fd4ca8-f270-42a4-aefb-d3396747ecfe
📒 Files selected for processing (10)
code/builders/builder-vite/input/iframe.htmlcode/builders/builder-vite/src/transform-iframe-html.tscode/builders/builder-webpack5/src/preview/iframe-webpack.config.tscode/builders/builder-webpack5/templates/preview.ejscode/core/assets/server/template.ejscode/core/src/builder-manager/index.tscode/core/src/builder-manager/utils/data.tscode/core/src/builder-manager/utils/template.tscode/core/src/core-server/presets/common-preset.tscode/core/src/types/modules/core-common.ts
mehm8128
force-pushed
the
feat/html-document-lang-option
branch
from
70f90ab to
9d39a5a
Compare
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
close #11706
related to #15541
What I did
I added the option to allow to change HTML lang attribute.
This option is essential for WCAG2.2 Success Criterion 3.1.1: Language of Page.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
yarn task --task sandbox --start-from auto --template react-vite/default-tshtmlLangoption tomain.tsof your sandbox.for example
<html lang="ja">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 publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit