feat(time-field,time-picker,scroll-fade): new tedi-ready components #374#397
Open
mart-sessman wants to merge 11 commits into
Open
feat(time-field,time-picker,scroll-fade): new tedi-ready components #374#397mart-sessman wants to merge 11 commits into
mart-sessman wants to merge 11 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces three new Angular components ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Input
participant TimeField as TimeFieldComponent
participant Parser as Validation/Parsing
participant Picker as TimePickerComponent
participant FormControl as FormControl
participant OnChange as onChange Handler
User->>TimeField: Input text (e.g., "14:30")
TimeField->>TimeField: blur event triggered
TimeField->>Parser: Validate HH:mm format
Parser-->>TimeField: Valid/Invalid result
alt Valid Input
TimeField->>TimeField: Update internal value
TimeField->>FormControl: Sync via writeValue
TimeField->>OnChange: Emit value change
else Invalid Input
TimeField->>TimeField: Revert to previous value
end
User->>TimeField: Click picker trigger
TimeField->>Picker: Open time picker
Picker->>Picker: Render scroll/dropdown/slots variant
User->>Picker: Select time (e.g., click hour 14)
Picker->>Picker: Update selected hour/minute
Picker->>OnChange: Emit value change
Picker->>FormControl: Sync value
alt closeOnSelect enabled
Picker->>TimeField: Close popover
end
TimeField->>TimeField: Update input display with selected time
sequenceDiagram
participant User as User/Content
participant ScrollFade as ScrollFadeComponent
participant Inner as Inner Wrapper div
participant Mask as CSS Mask-Image
participant Output as scrolledToTop/Bottom Output
User->>ScrollFade: Provide scrollable content
ScrollFade->>Inner: Set overflow: auto
Inner->>Mask: Apply linear-gradient mask-image
Mask-->>Inner: Create fade effect at edges
User->>Inner: Scroll down
Inner->>ScrollFade: Dispatch scroll event
ScrollFade->>ScrollFade: Calculate scroll position
alt At Bottom
ScrollFade->>Output: Emit scrolledToBottom
else At Top
ScrollFade->>Output: Emit scrolledToTop
else Middle
ScrollFade->>ScrollFade: Update fade classes dynamically
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
mart-sessman
temporarily deployed
to
github-pages
GitHub Actions
Inactive
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tedi/components/form/index.ts (1)
9-15:⚠️ Potential issue | 🟡 MinorDuplicate export of
date-picker.component.Lines 9 and 15 both export
"./date-picker/date-picker.component". This will cause a TypeScript compilation error or unexpected behavior.🐛 Proposed fix
export * from "./radio-card-group/radio-card-group.component"; export * from "./date-picker/date-picker.component"; export * from "./feedback-text/feedback-text.component"; export * from "./label/label.component"; export * from "./number-field/number-field.component"; export * from "./select"; export * from "./toggle/toggle.component"; -export * from "./date-picker/date-picker.component"; export * from "./form-field/form-field.component";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/form/index.ts` around lines 9 - 15, There are duplicate exports of "./date-picker/date-picker.component" in the form barrel; remove the redundant export so the module is only exported once (keep a single export line for "./date-picker/date-picker.component" and delete the other), and verify the remaining exports (e.g., "./feedback-text/feedback-text.component", "./label/label.component", "./number-field/number-field.component", "./select", "./toggle/toggle.component") are correct.
🧹 Nitpick comments (5)
tedi/components/form/time-field/time-field.component.scss (1)
53-62: Remove!importantdeclarations to resolve specificity issues.The
!importantoverrides on lines 54–57 indicate specificity conflicts with base button styles and can lead to maintenance issues. Consider:
- Increasing selector specificity naturally (e.g.,
.tedi-time-field .tedi-time-field__icon)- Using CSS custom properties that the button component respects
- Adjusting the base button component to be more customizable
The
tedi-time-field__icon--staticmodifier class used in the HTML template (non-interactive state) does not have explicit styles, but this appears intentional—it's a semantic marker that correctly inherits from the base&__iconstyles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/form/time-field/time-field.component.scss` around lines 53 - 62, Remove the four `!important` overrides on the `&__icon` rules to avoid specificity fights; instead increase selector specificity (e.g., target `.tedi-time-field .tedi-time-field__icon`) or rely on configurable CSS custom properties the base button consumes (use the existing --button-xs-icon-size, --form-field-button-height-sm, --button-radius-sm variables) so the icon styles inherit normally; keep the `tedi-time-field__icon--static` modifier as-is since it intentionally inherits from `&__icon`.tedi/components/helpers/scroll-fade/scroll-fade.component.scss (1)
26-44: CSS nesting depth exceeds maximum of 4 levels.The
:hoverpseudo-class inside&::-webkit-scrollbar-thumbcreates 5 levels of nesting (.tedi-scroll-fade→&__inner→&--custom-scroll→&::-webkit-scrollbar-thumb→&:hover). As per coding guidelines, max CSS nesting depth is 4.♻️ Proposed fix to flatten nesting
&__inner { flex: 1; min-height: 0; max-height: inherit; overflow: auto; mask-image: linear-gradient( to bottom, transparent 0%, black var(--_tedi-scroll-fade-top), black calc(100% - var(--_tedi-scroll-fade-bottom)), transparent 100% ); - - &--custom-scroll { - &::-webkit-scrollbar { - width: 6px; - background-color: var(--general-surface-primary); - } - - &::-webkit-scrollbar-thumb { - background: var(--general-border-primary); - border-radius: 100px; - - &:hover { - background-color: var(--general-border-secondary); - } - } - - &::-webkit-scrollbar-track { - background-color: var(--general-surface-primary); - } - } } + + &__inner--custom-scroll { + &::-webkit-scrollbar { + width: 6px; + background-color: var(--general-surface-primary); + } + + &::-webkit-scrollbar-thumb { + background: var(--general-border-primary); + border-radius: 100px; + } + + &::-webkit-scrollbar-thumb:hover { + background-color: var(--general-border-secondary); + } + + &::-webkit-scrollbar-track { + background-color: var(--general-surface-primary); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/helpers/scroll-fade/scroll-fade.component.scss` around lines 26 - 44, The nesting depth exceeds the max because the &:hover pseudo inside &::-webkit-scrollbar-thumb creates five levels; move the hover selector out to flatten it. Concretely, remove the nested &:hover inside &::-webkit-scrollbar-thumb and add a top-level selector targeting &--custom-scroll::-webkit-scrollbar-thumb:hover (or the equivalent flattened selector using &--custom-scroll and ::-webkit-scrollbar-thumb:hover) so you keep the same styles but reduce nesting; update selectors referencing &--custom-scroll, &::-webkit-scrollbar-thumb, and &:hover accordingly.tedi/components/form/time-field/time-field.component.html (1)
29-49: Consider moving inline styles to CSS classes.Lines 31 and 49 use inline
styleattributes. For consistency with the BEM approach and maintainability, consider using CSS classes instead.♻️ Suggested approach
Add classes to the SCSS file:
.tedi-time-field__popover-trigger { display: flex; align-items: center; } .tedi-time-field__popover-content { padding: 0; }Then update the template:
<tedi-popover `#popover` - style="display: flex; align-items: center" + class="tedi-time-field__popover-trigger" position="bottom-end" ... - <tedi-popover-content maxWidth="none" style="padding: 0"> + <tedi-popover-content maxWidth="none" class="tedi-time-field__popover-content">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/form/time-field/time-field.component.html` around lines 29 - 49, Replace the inline styles on the tedi-popover host element and tedi-popover-content with CSS classes: create classes (e.g., .tedi-time-field__popover-trigger and .tedi-time-field__popover-content) in the component SCSS containing the styles "display: flex; align-items: center" and "padding: 0" respectively, then remove the style attributes from the <tedi-popover `#popover` ...> and <tedi-popover-content ...> elements and add the corresponding class names to those elements to follow the BEM approach and keep styling in SCSS.tedi/components/form/time-field/time-field.component.ts (1)
194-203: NestedsetTimeoutpattern is fragile for timing coordination.The double
setTimeoutrelies on specific timing to wait for popover rendering and focus setup. This could be flaky if rendering takes longer than expected.Consider using a more robust approach such as observing DOM changes or using Angular's lifecycle hooks if possible.
♻️ Consider using requestAnimationFrame or AfterViewInit pattern
onPickerOpen() { - // First timeout waits for popover to render content in DOM. - // Nested timeout runs after popover's own focus setup (which also uses setTimeout). - setTimeout(() => { - this.timePicker()?.scrollToSelected(); - setTimeout(() => { - this.timePicker()?.focusActiveItem(); - }); - }); + // Use requestAnimationFrame to ensure DOM is ready + requestAnimationFrame(() => { + this.timePicker()?.scrollToSelected(); + requestAnimationFrame(() => { + this.timePicker()?.focusActiveItem(); + }); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/form/time-field/time-field.component.ts` around lines 194 - 203, The nested setTimeouts in onPickerOpen (which calls this.timePicker()?.scrollToSelected() and this.timePicker()?.focusActiveItem()) are fragile; replace them with a more robust coordination such as waiting for Angular stabilization or the next animation frame: e.g., use NgZone.onStable or requestAnimationFrame (or a MutationObserver if the popover content is dynamic) to call scrollToSelected() after the popover DOM is rendered and then schedule focusActiveItem() on the next animation frame (or when the zone is stable) instead of using nested setTimeouts.tedi/components/helpers/scroll-fade/scroll-fade.component.ts (1)
76-99: Consider using an arrow function foronScrollto ensure stable reference.While
onScrollis currently called via template binding(scroll)="onScroll()"which works correctly, the coding guidelines recommend arrow function properties for event handlers. This is a minor suggestion since the current implementation is functional.♻️ Optional: Convert to arrow function
- onScroll(): void { + onScroll = (): void => { const el = this.innerRef().nativeElement; this.updateFade(el.scrollTop, el.scrollHeight, el.clientHeight); - } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tedi/components/helpers/scroll-fade/scroll-fade.component.ts` around lines 76 - 99, The onScroll method should be converted to an arrow-function property to ensure a stable function reference for template bindings; replace the class method onScroll() with an arrow property (keeping the same logic that reads const el = this.innerRef().nativeElement and calls this.updateFade) so it becomes onScroll = (): void => { ... }, leaving updateFade, ngAfterViewInit, fade, scrolledToTop and scrolledToBottom unchanged; ensure any places that call onScroll (template bindings) remain unchanged since the signature and behavior are identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tedi/components/form/time-picker/time-picker.component.html`:
- Around line 7-9: Replace the hardcoded aria-label attributes in the
time-picker template with the tediTranslate pipe: update the listbox/role
elements that currently use aria-label="Hours" and aria-label="Minutes" to use
the translation pipe (e.g., aria-label="{{ 'timePicker.hours' | tediTranslate
}}") so the labels are localized; ensure you update the template instances in
time-picker.component.html and add the corresponding translation keys (e.g.,
timePicker.hours, timePicker.minutes) to the translations map used by the
tediTranslate service.
---
Outside diff comments:
In `@tedi/components/form/index.ts`:
- Around line 9-15: There are duplicate exports of
"./date-picker/date-picker.component" in the form barrel; remove the redundant
export so the module is only exported once (keep a single export line for
"./date-picker/date-picker.component" and delete the other), and verify the
remaining exports (e.g., "./feedback-text/feedback-text.component",
"./label/label.component", "./number-field/number-field.component", "./select",
"./toggle/toggle.component") are correct.
---
Nitpick comments:
In `@tedi/components/form/time-field/time-field.component.html`:
- Around line 29-49: Replace the inline styles on the tedi-popover host element
and tedi-popover-content with CSS classes: create classes (e.g.,
.tedi-time-field__popover-trigger and .tedi-time-field__popover-content) in the
component SCSS containing the styles "display: flex; align-items: center" and
"padding: 0" respectively, then remove the style attributes from the
<tedi-popover `#popover` ...> and <tedi-popover-content ...> elements and add the
corresponding class names to those elements to follow the BEM approach and keep
styling in SCSS.
In `@tedi/components/form/time-field/time-field.component.scss`:
- Around line 53-62: Remove the four `!important` overrides on the `&__icon`
rules to avoid specificity fights; instead increase selector specificity (e.g.,
target `.tedi-time-field .tedi-time-field__icon`) or rely on configurable CSS
custom properties the base button consumes (use the existing
--button-xs-icon-size, --form-field-button-height-sm, --button-radius-sm
variables) so the icon styles inherit normally; keep the
`tedi-time-field__icon--static` modifier as-is since it intentionally inherits
from `&__icon`.
In `@tedi/components/form/time-field/time-field.component.ts`:
- Around line 194-203: The nested setTimeouts in onPickerOpen (which calls
this.timePicker()?.scrollToSelected() and this.timePicker()?.focusActiveItem())
are fragile; replace them with a more robust coordination such as waiting for
Angular stabilization or the next animation frame: e.g., use NgZone.onStable or
requestAnimationFrame (or a MutationObserver if the popover content is dynamic)
to call scrollToSelected() after the popover DOM is rendered and then schedule
focusActiveItem() on the next animation frame (or when the zone is stable)
instead of using nested setTimeouts.
In `@tedi/components/helpers/scroll-fade/scroll-fade.component.scss`:
- Around line 26-44: The nesting depth exceeds the max because the &:hover
pseudo inside &::-webkit-scrollbar-thumb creates five levels; move the hover
selector out to flatten it. Concretely, remove the nested &:hover inside
&::-webkit-scrollbar-thumb and add a top-level selector targeting
&--custom-scroll::-webkit-scrollbar-thumb:hover (or the equivalent flattened
selector using &--custom-scroll and ::-webkit-scrollbar-thumb:hover) so you keep
the same styles but reduce nesting; update selectors referencing
&--custom-scroll, &::-webkit-scrollbar-thumb, and &:hover accordingly.
In `@tedi/components/helpers/scroll-fade/scroll-fade.component.ts`:
- Around line 76-99: The onScroll method should be converted to an
arrow-function property to ensure a stable function reference for template
bindings; replace the class method onScroll() with an arrow property (keeping
the same logic that reads const el = this.innerRef().nativeElement and calls
this.updateFade) so it becomes onScroll = (): void => { ... }, leaving
updateFade, ngAfterViewInit, fade, scrolledToTop and scrolledToBottom unchanged;
ensure any places that call onScroll (template bindings) remain unchanged since
the signature and behavior are identical.
🪄 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: CHILL
Plan: Pro
Run ID: 6e783f1b-a5bc-4af9-8e0b-aae44c0c937c
📒 Files selected for processing (23)
.claude/skills/contributing/references/best-practices.mdtedi/components/form/form-field/form-field.component.htmltedi/components/form/index.tstedi/components/form/time-field/index.tstedi/components/form/time-field/time-field.component.htmltedi/components/form/time-field/time-field.component.scsstedi/components/form/time-field/time-field.component.spec.tstedi/components/form/time-field/time-field.component.tstedi/components/form/time-field/time-field.stories.tstedi/components/form/time-picker/index.tstedi/components/form/time-picker/time-picker.component.htmltedi/components/form/time-picker/time-picker.component.scsstedi/components/form/time-picker/time-picker.component.spec.tstedi/components/form/time-picker/time-picker.component.tstedi/components/form/time-picker/time-picker.stories.tstedi/components/helpers/index.tstedi/components/helpers/scroll-fade/index.tstedi/components/helpers/scroll-fade/scroll-fade.component.htmltedi/components/helpers/scroll-fade/scroll-fade.component.scsstedi/components/helpers/scroll-fade/scroll-fade.component.spec.tstedi/components/helpers/scroll-fade/scroll-fade.component.tstedi/components/helpers/scroll-fade/scroll-fade.stories.tstedi/services/translation/translations.ts
mart-sessman
temporarily deployed
to
github-pages
GitHub Actions
Inactive
mart-sessman
temporarily deployed
to
github-pages
GitHub Actions
Inactive
Contributor
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Approval is disabled; enable |
…cker-tedi-ready # Conflicts: # tedi/components/helpers/scroll-fade/scroll-fade.component.scss # tedi/components/helpers/scroll-fade/scroll-fade.component.ts
mart-sessman
temporarily deployed
to
github-pages
GitHub Actions
Inactive
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
1 participant
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 by CodeRabbit
Release Notes