feat: add valuePrefix and valueSuffix properties as replacements for deprecated prefix and suffix#3969
Open
adrianschmidt-bot wants to merge 7 commits intoLundalogik:mainfrom
Conversation
…deprecated prefix and suffix This introduces new 'valuePrefix' and 'valueSuffix' properties for limel-circular-progress, limel-info-tile, and limel-input-field components to resolve conflicts with the native HTML 'prefix' attribute (RDFa global attribute). The new properties follow the same behavior as the existing ones while providing clearer semantics. The old 'prefix' and 'suffix' properties remain functional but are marked as deprecated with runtime warnings to guide consumers toward migration. Changes: - Add valuePrefix and valueSuffix props to all three components with reflection support - Update InfoTileProgress interface with new properties - Implement fallback logic (valuePrefix ?? prefix) throughout components - Mark old properties with @deprecated JSDoc tags - Add console.warn for deprecated property usage with migration guidance - Update all examples and documentation to use new property names fix: Lundalogik#3917
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/components/info-tile/info-tile.tsx (1)
109-114:⚠️ Potential issue | 🟡 MinorUpdate the
progressdocs to usevalueSuffix.The defaults block still points readers at
suffix, so the public API docs will keep advertising the deprecated field instead of the new one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/info-tile/info-tile.tsx` around lines 109 - 114, The JSDoc for the progress prop still documents the deprecated `suffix` instead of the new `valueSuffix`; update the defaults block in the `progress` docs to reference `valueSuffix` (and remove or mark `suffix` as deprecated) so generated public API docs advertise `valueSuffix` correctly; look for the `progress` prop JSDoc in info-tile (info-tile.tsx) and replace `suffix` with `valueSuffix` in the Defaults list (and ensure `displayPercentageColors`/`maxValue` remain correct).src/components/circular-progress/circular-progress.tsx (1)
113-124:⚠️ Potential issue | 🟠 MajorDon't hardcode the progressbar label to percent.
With the new
valueSuffixexamples covering degrees, seconds, stars, and other units,aria-label="%"on Line 120 will misannounce non-percent scales to assistive tech. Prefer an accessible value/label derived fromeffectivePrefix,value, andeffectiveSuffix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/circular-progress/circular-progress.tsx` around lines 113 - 124, The ARIA label is hardcoded to "%" which misrepresents non-percent units; update the circular-progress element to build an accessible label/text from effectivePrefix, this.value, and effectiveSuffix (use the already-computed effectivePrefix and effectiveSuffix variables and this.value) and set that string to aria-label (or aria-valuetext) instead of "%" so assistive tech announces the correct unit and value; ensure you update the JSX/TSX return where class={classList} and role="progressbar" are set.src/components/info-tile/examples/info-tile-styling.tsx (1)
28-45: 🛠️ Refactor suggestion | 🟠 MajorUse
<Host>instead of returning an array literal fromrender().Multiple top-level JSX elements must be wrapped in the
<Host>element, not an array literal.public render() { return ( <Host> <limel-info-tile icon="electricity" label="Average weekly usage" value={this.value} valueSuffix="kWh" badge={this.badge} /> <limel-info-tile label="Average weekly usage" icon="electricity" value={this.value} valueSuffix="kWh" progress={this.progress} valuePrefix="↑" /> </Host> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/info-tile/examples/info-tile-styling.tsx` around lines 28 - 45, The render method currently returns an array of two top-level JSX elements; replace that array return in the render() function with a single <Host> wrapper that contains both <limel-info-tile> elements so multiple top-level nodes are valid; update the render() implementation that references this.value, this.badge and this.progress to return a single <Host> containing the two <limel-info-tile> instances instead of returning an array literal.src/components/circular-progress/examples/circular-progress-props.tsx (1)
34-61:⚠️ Potential issue | 🟡 MinorUse
<Host>instead of returning[]fromrender().The render method returns multiple top-level elements wrapped in an array literal. Per the coding guidelines, wrap them in
<Host>(imported from@stencil/core) instead. AddHostto the import statement and refactor the render method accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/circular-progress/examples/circular-progress-props.tsx` around lines 34 - 61, The render() method currently returns an array of limel-circular-progress elements; change it to return a single <Host> wrapper containing those elements and add Host to the import from `@stencil/core` (update the import statement to include Host) so the JSX uses <Host>...</Host> instead of returning [] in render().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/circular-progress/circular-progress.tsx`:
- Around line 96-111: Move the deprecation console.warn calls out of render()
into a lifecycle hook (componentWillLoad or componentDidLoad) so they execute
only once per instance; specifically check this.prefix and the deprecated
this.suffix logic there (preserve the same conditions) and remove them from
render(). Also update the aria-label usage in the render output (currently
hardcoded as "%") to use the computed effectiveSuffix (e.g., use effectiveSuffix
or a derived variable instead of "%") so the screen reader reflects valueSuffix
when provided.
In `@src/components/info-tile/info-tile.tsx`:
- Around line 299-311: The component is passing computed
effectiveProgressPrefix/effectiveProgressSuffix into both the new
valuePrefix/valueSuffix and the deprecated prefix/suffix props, which forces
migrated callers through the deprecation path; change the JSX for
limel-circular-progress so valuePrefix/valueSuffix use
effectiveProgressPrefix/effectiveProgressSuffix, but the legacy prefix/suffix
props receive the raw originals (this.progress.prefix and this.progress.suffix)
or are omitted when undefined—stop passing
effectiveProgressPrefix/effectiveProgressSuffix into the deprecated
prefix/suffix attributes to avoid nested deprecation handling.
- Around line 143-153: The deprecation console.warn calls in render() (for the
prefix and suffix props) and in renderProgress() are firing on every render;
remove those console.warns from both render methods and instead emit the
warnings once during component initialization or on prop change by implementing
them in componentWillLoad() or with `@Watch`() handlers. Specifically, move the
prefix/suffix warnings out of render() into componentWillLoad(), or add
`@Watch`('prefix') and `@Watch`('suffix') methods that log when the value becomes
non-null/undefined; do the same for the progress-related deprecation warnings
currently in renderProgress() (implement `@Watch` handlers or check once in
componentWillLoad()), and ensure to import and use the Watch decorator if you
choose that approach.
In `@src/components/input-field/input-field.tsx`:
- Around line 363-373: Move the deprecation console.warn calls out of render()
and into a lifecycle hook that runs once per component instance (e.g.,
componentWillLoad()) to avoid logging on every re-render; locate the checks
referencing this.prefix and this.suffix in render() and relocate them to
componentWillLoad() (or add an instance-level boolean like hasLoggedDeprecation
to guard logging) and keep the same messages recommending
valuePrefix/valueSuffix so warnings are emitted only once.
---
Outside diff comments:
In `@src/components/circular-progress/circular-progress.tsx`:
- Around line 113-124: The ARIA label is hardcoded to "%" which misrepresents
non-percent units; update the circular-progress element to build an accessible
label/text from effectivePrefix, this.value, and effectiveSuffix (use the
already-computed effectivePrefix and effectiveSuffix variables and this.value)
and set that string to aria-label (or aria-valuetext) instead of "%" so
assistive tech announces the correct unit and value; ensure you update the
JSX/TSX return where class={classList} and role="progressbar" are set.
In `@src/components/circular-progress/examples/circular-progress-props.tsx`:
- Around line 34-61: The render() method currently returns an array of
limel-circular-progress elements; change it to return a single <Host> wrapper
containing those elements and add Host to the import from `@stencil/core` (update
the import statement to include Host) so the JSX uses <Host>...</Host> instead
of returning [] in render().
In `@src/components/info-tile/examples/info-tile-styling.tsx`:
- Around line 28-45: The render method currently returns an array of two
top-level JSX elements; replace that array return in the render() function with
a single <Host> wrapper that contains both <limel-info-tile> elements so
multiple top-level nodes are valid; update the render() implementation that
references this.value, this.badge and this.progress to return a single <Host>
containing the two <limel-info-tile> instances instead of returning an array
literal.
In `@src/components/info-tile/info-tile.tsx`:
- Around line 109-114: The JSDoc for the progress prop still documents the
deprecated `suffix` instead of the new `valueSuffix`; update the defaults block
in the `progress` docs to reference `valueSuffix` (and remove or mark `suffix`
as deprecated) so generated public API docs advertise `valueSuffix` correctly;
look for the `progress` prop JSDoc in info-tile (info-tile.tsx) and replace
`suffix` with `valueSuffix` in the Defaults list (and ensure
`displayPercentageColors`/`maxValue` remain correct).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83baefa3-5576-4052-aa06-870a713a0548
📒 Files selected for processing (13)
src/components/circular-progress/circular-progress.tsxsrc/components/circular-progress/examples/circular-progress-props.tsxsrc/components/info-tile/examples/info-tile-badge.tsxsrc/components/info-tile/examples/info-tile-loading.tsxsrc/components/info-tile/examples/info-tile-primary-slot.tsxsrc/components/info-tile/examples/info-tile-progress.tsxsrc/components/info-tile/examples/info-tile-styling.tsxsrc/components/info-tile/examples/info-tile.tsxsrc/components/info-tile/info-tile.tsxsrc/components/info-tile/info-tile.types.tssrc/components/input-field/examples/input-field-number-prefix.tsxsrc/components/input-field/examples/input-field-text-suffix.tsxsrc/components/input-field/input-field.tsx
…ts for deprecated prefix and suffix address CodeRabbit review: move deprecation warnings to lifecycle hooks, fix aria-label, update JSDoc, fix nested deprecation
Contributor
Author
CodeRabbit review — addressedFixed (⚡ 32ceb09):
Declined — |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/circular-progress/circular-progress.tsx`:
- Around line 63-73: The props valuePrefix and valueSuffix have inconsistent
defaults (valuePrefix is explicitly initialized to null while valueSuffix is
left undefined); to align them, make their defaults consistent—e.g., set
valueSuffix to null (or change valuePrefix to undefined) so both props use the
same sentinel value and the existing checks (!== null && !== undefined) remain
correct; update the declarations for valuePrefix/valueSuffix accordingly
(referencing the Prop-decorated fields valuePrefix and valueSuffix in
circular-progress.tsx).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: abe0ce48-a589-46ed-ad02-d5b67eaf43ae
📒 Files selected for processing (3)
src/components/circular-progress/circular-progress.tsxsrc/components/info-tile/info-tile.tsxsrc/components/input-field/input-field.tsx
…ts for deprecated prefix and suffix fixup! feat: add valuePrefix and valueSuffix properties as replacements for deprecated prefix and suffix Make valuePrefix default consistent with valueSuffix (both undefined)
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/circular-progress/circular-progress.tsx`:
- Around line 66-73: The markdown whitelist is missing the reflected attribute
names for the circular progress component: add "value-prefix" and "value-suffix"
to the allowed attributes for limel-circular-progress in
src/components/markdown/default-whitelist.ts while keeping the existing "prefix"
and "suffix" entries for the deprecation period; update the whitelist entries so
both the new reflected attribute names (value-prefix/value-suffix) and the old
names (prefix/suffix) are present and accepted for limel-circular-progress,
ensuring the component properties valuePrefix and valueSuffix continue to work
when rendered from markdown.
- Around line 63-67: The example still uses the deprecated prefix attribute on
the limel-circular-progress tag; update the example to use the new valuePrefix
prop instead. Locate the example that renders <limel-circular-progress
prefix="↗" ...> and change the attribute to valuePrefix="↗", and update any
surrounding example text/comment that describes the prop to mention valuePrefix
(and demonstrate its behavior) so the docs no longer reference the deprecated
API and show the new prop usage. Ensure the component usage and wording in the
example match the new `@Prop` public valuePrefix?: string declaration.
- Around line 101-108: The deprecation guard in warnIfDeprecatedSuffix currently
suppresses the warning when valueSuffix is present; update the conditional in
the `@Watch`('suffix') method (warnIfDeprecatedSuffix) to remove the
"this.valueSuffix === undefined" clause so the warning still triggers when a
user explicitly sets suffix (except for the implicit '%' case). Keep the
existing checks for this.suffix !== '%' and null/undefined checks, and ensure
the method emits the deprecation warning when suffix is explicitly provided even
if valueSuffix is also set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d454498-f091-4335-852a-f4cb52862bb2
📒 Files selected for processing (1)
src/components/circular-progress/circular-progress.tsx
…ts for deprecated prefix and suffix fixup! feat: add valuePrefix and valueSuffix properties as replacements for deprecated prefix and suffix - Update circular-progress CSS variables example to use valuePrefix - Add value-prefix/value-suffix to markdown whitelist for both circular-progress and info-tile - Remove valueSuffix===undefined guard from suffix deprecation warning so warning fires when both suffix and valueSuffix are set
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
…ts for deprecated prefix and suffix fixup! feat: add valuePrefix and valueSuffix properties as replacements for deprecated prefix and suffix Initialize valuePrefix to null and valueSuffix to '%' in circular-progress to match the defaults of the deprecated prefix/suffix properties. Update fallback logic so the deprecated suffix prop still takes precedence when explicitly set by consumers.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/circular-progress/circular-progress.tsx`:
- Around line 49-54: Fix the JSDoc typo above the prefix prop: in the comment
block for the public prefix?: string = null; (the JSDoc that starts with "The
prefix which is displayed before the `value`"), remove the duplicated word
"characters" so the sentence reads "must be a few characters long."; keep the
deprecation note and `@Prop`({ reflect: true }) decorator unchanged.
- Around line 63-67: Fix the JSDoc for the valuePrefix prop by removing the
duplicated word "characters" and making the sentence concise; update the comment
above the `@Prop` declaration for valuePrefix (the public valuePrefix?: string =
null;) so it reads something like "The prefix which is displayed before the
`value`, must be a few characters long." or another concise variant without the
repeated word.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4c91df8-1bb3-4a67-8b8f-1bf70f337fa1
📒 Files selected for processing (1)
src/components/circular-progress/circular-progress.tsx
…ts for deprecated prefix and suffix fix JSDoc typo: duplicated word 'characters'
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/circular-progress/circular-progress.tsx`:
- Around line 123-127: The prefix and suffix fallback logic are inconsistent:
effectivePrefix gives the new prop (valuePrefix) precedence while
effectiveSuffix favors the deprecated prop (suffix) when changed from default;
update the suffix resolution so the new prop always wins—i.e., in the
calculation for effectiveSuffix (the same area that defines effectivePrefix and
effectiveSuffix) check this.valueSuffix first and return it when explicitly set,
otherwise fall back to the deprecated this.suffix (with the existing '%'
default-handling) and finally to the valueSuffix default; adjust the logic
around the effectiveSuffix variable to mirror the precedence used by
effectivePrefix while preserving the special-case '%' behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6229d60f-1c5d-44b0-b2bd-f78b5e3ff861
📒 Files selected for processing (1)
src/components/circular-progress/circular-progress.tsx
…ts for deprecated prefix and suffix
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3969/ |
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.
Summary
This PR implements Phase 1 of issue #3917 by introducing new
valuePrefixandvalueSuffixproperties for the three affected components while maintaining backward compatibility.What's Changed
🎯 Components Updated
valuePrefixandvalueSuffixpropsvaluePrefixandvalueSuffixpropsvaluePrefixandvalueSuffixpropsvaluePrefixandvalueSuffixproperties⚡ Implementation Details
this.valuePrefix ?? this.prefix)value-prefixandvalue-suffixattributesconsole.warnmessages guide developers to migrate@deprecatedtags📚 Documentation & Examples
Why This Approach?
The new
valuePrefix/valueSuffixnames clearly indicate their purpose (prefixing/suffixing the value display) while avoiding the collision with the native HTMLprefixattribute used by RDFa.Testing
Migration Path
Developers can migrate at their own pace:
Closes #3917 (Phase 1)
Summary by CodeRabbit
New Features
Deprecations
Accessibility
Examples
Chores