feat: custom modal dimension #1087
Conversation
This reverts commit 8bf3da5.
Codecov Report
@@ Coverage Diff @@
## main #1087 +/- ##
==========================================
+ Coverage 85.12% 85.13% +0.01%
==========================================
Files 821 821
Lines 16965 16993 +28
Branches 2044 2056 +12
==========================================
+ Hits 14441 14467 +26
- Misses 2493 2495 +2
Partials 31 31
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| content: TemplateRef<unknown> | Type<unknown>; | ||
| size?: ModalSize; |
There was a problem hiding this comment.
it should only be size OR dimension right? can we put them into the same parameter (alternatively, using a union - but seems like overkill). Basically just size: ModalSize | ModalDimension
There was a problem hiding this comment.
Not sure if we need to make these many changes. Can we follow the approach in with Sheet module. It has a responsive sheet size enum, can we build something similar here?
&.sheet-size-responsive-extra-large {
width: 100%;
}
check hypertrace-ui/projects/components/src/overlay/sheet/sheet-overlay.component.ts
There was a problem hiding this comment.
ofcourse, above class would have height: 100% as well and we would be setting 100% for width and height in popover trigger
There was a problem hiding this comment.
&.sheet-size-responsive-extra-large {
width: 100%;
}
This is for full size , right?
But it is different from custom size?
There was a problem hiding this comment.
it should only be size OR dimension right? can we put them into the same parameter (alternatively, using a union - but seems like overkill). Basically just
size: ModalSize | ModalDimension
For this, is there any way for us to check, value is type of ModalSize or ModalDimension
There was a problem hiding this comment.
Yeah, but can we do something similar and let the content size drive the modal size? Meaning, we shouldn't need to pass width/height as parameters, but can the modal adjust its size based on the content dom dimensions?
|
how come you so many merge commits in this branch? :P |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| content: TemplateRef<unknown> | Type<unknown>; | ||
| size: ModalSize | ModalDimension; |
There was a problem hiding this comment.
hmm... this kind of makes the design weird. The same property is trying to do two things.
What if, we introduce a new enum in ModalSize called CustomSize and then add another optional property for dimension inputs? Then we can keep the code cleaner.
Alternatively, the config itself can change for custom size.
@aaron-steinfeld thoughts?
There was a problem hiding this comment.
I disagree. It's serving the same role in either case - a way to specify the size. Every other option is the same, so it doesn't make sense to have two versions of the config or an optional property (since the optional property is meaningless depending on the enum value). This is the beauty of unions - it's a strictly easier API for the caller to work with. Any messiness is internal to the implementation, and disambiguating between types should really just be a one liner.
There was a problem hiding this comment.
fine. I don't have strong feelings. I find it weird when we put type based logic in the code. IMO, keeping separate properties makes it cleaner from API perspective. But it is a preference
This comment has been minimized.
This comment has been minimized.
| @@ -19,6 +19,33 @@ export const enum ModalSize { | |||
| MediumWide = 'medium-wide' | |||
| } | |||
|
|
|||
| export interface ModalDimension { | |||
| // Number => without unit (considered px) and String => with units (expression included) | |||
| width: number | string; | |||
There was a problem hiding this comment.
What if I pass a random string?
There was a problem hiding this comment.
I mean, it won't work at that time. And I think this should be the case. Like suppose if we do this in the css (putting wrong value for width), then it doesn't work. So It is developer's responsibility to put correct value and I think developer can handle this.
There was a problem hiding this comment.
If you want to get really fancy, TS supports this kind of thing now. I'd also be fine just taking in any string, we don't validate to nearly this level elsewhere.
type Unit = "px" | "em" | "fr" | "%"; // probably some others too
type SizeWithUnit = `${number}${Unit}`;
There was a problem hiding this comment.
Yeah, we could do this, but my intention of using string, to use these kind of expression as well, not only simple units - calc(100vh - 100px)
There was a problem hiding this comment.
type SizeExpression = SizeWithUnit | `calc(${SizeWithUnit} - ${SizeWithUnit})`Typescript type system is turing complete 😉
But seriously, yes I agree, overkill.
There was a problem hiding this comment.
I will let you guys decide :D
There was a problem hiding this comment.
According to me, It is good enough and as @aaron-steinfeld said doing this, is an overkill.
I think we can merge this.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Description
This will give the flexibility to use custom dimensions for modal.
width/height can be set as expression as well -
calc(100vw - 100px)