fix(popover): set default Radix values #189
Conversation
pimenovoleg
commented
Dec 12, 2024
•
pimenovoleg
commented
- Set default values: https://www.radix-ui.com/primitives/docs/components/popover#content
- Added attrs transformation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@pawel-twardziak look on this |
This reverts commit 47f3298.
| @@ -47,7 +50,7 @@ export class RdxPopoverContentDirective implements OnInit { | |||
| /** | |||
| * The distance in pixels from the trigger. | |||
| */ | |||
| readonly sideOffset = input<number | undefined>(void 0); | |||
There was a problem hiding this comment.
it is intentional to have void 0 as a default value.
There was a problem hiding this comment.
because the offsets are calculated as a fallback later on unless any number value is provided.
Angular will not allow to provide any other value that is of other than numer or undefined type.
There was a problem hiding this comment.
the change will break that functionality
| @@ -38,12 +40,12 @@ export class RdxPopoverArrowDirective implements AfterViewInit { | |||
| /** | |||
| * The width of the arrow in pixels. | |||
| */ | |||
| readonly width = input<number>(10); | |||
| readonly width = input<number, NumberInput>(10, { transform: numberAttribute }); | |||
There was a problem hiding this comment.
this is not necessary. I've never seen the usage of it before for such cases.
Angular inputs are typed, there is no point in having that transformer here :)
|
|
||
| /** | ||
| * Whether to add some alternate positions of the content. | ||
| */ | ||
| readonly disableAlternatePositions = input(false); | ||
| readonly disableAlternatePositions = input<boolean, BooleanInput>(false, { transform: booleanAttribute }); |
There was a problem hiding this comment.
there is no point in having that transformer here - unnecessary code that brings no value imho
| @@ -85,7 +88,7 @@ export class RdxPopoverContentDirective implements OnInit { | |||
| * Alternate positions for better user experience along the X/Y axis (e.g. vertical/horizontal scrolling) | |||
| */ | |||
| const allPossibleConnectedPositions = getAllPossibleConnectedPositions(); | |||
| allPossibleConnectedPositions.forEach((value, key) => { | |||
| allPossibleConnectedPositions.forEach((_value, key) => { | |||
There was a problem hiding this comment.
below, there is a piece of code to fix.
that:
const offsets: RdxSideAndAlignOffsets = {
sideOffset: this.sideOffset() ?? (greatestDimensionFromTheArrow || DEFAULTS.offsets.side),
alignOffset: this.alignOffset() ?? (greatestDimensionFromTheArrow || DEFAULTS.offsets.align)
};should be this:
const offsets: RdxSideAndAlignOffsets = {
sideOffset: this.sideOffset() ?? (greatestDimensionFromTheArrow || DEFAULTS.offsets.side),
alignOffset: this.alignOffset()
};it is because alignOffset should always be 0 by fefault. It is only the sideOffset that should get aligned to the arrow dimensions as fallback to void 0 or, if the arrow does not exist, to the default value then
There was a problem hiding this comment.
I've fixed this there -> #192
