The date picker in search filter is not positioned correctly [SDBELGA-995] #4978
Conversation
|
why did we do the padding change? Could you post before/after screenshots |
|
Before screenshot: After screenshot: |
The padding change was made to improve the spacing and overall layout consistency. |
tomaskikutis
left a comment
tomaskikutis
left a comment
There was a problem hiding this comment.
That's not fixing the root cause of the issue. It's positioning code of date picker that should be improved.
|
Tomas is correct, fix should be in UI framework, we have some positioning code already there for e.g. |
scripts/core/ui/ui.ts
Outdated
| if (value) { | ||
| $popupWrapper.offset(popupService.position(260, 270, element)); | ||
| scope.$broadcast('datepicker.focus'); | ||
| $timeout(() => { |
There was a problem hiding this comment.
why do you need $timeout from angular? you can use setTimeout probably.
Also why is the timeout needed in the first place? Are you waiting for the component to render first? Also can you drop the usage of this legacy $popupWrapper?
Could you instead of making a legacy hack replace the whole component with a new one from UI framework?
There was a problem hiding this comment.
I was thinking the same that it would probably be easier to connect date picker from ui-framework rather than correct positioning code which is not the easiest thing. But if this patch works I'm fine with the existing solution.
thecalcc
changed the title
scripts/core/ui/ui.ts
Outdated
| if (value) { | ||
| $popupWrapper.offset(popupService.position(260, 270, element)); | ||
| scope.$broadcast('datepicker.focus'); | ||
| $timeout(() => { |
There was a problem hiding this comment.
I was thinking the same that it would probably be easier to connect date picker from ui-framework rather than correct positioning code which is not the easiest thing. But if this patch works I'm fine with the existing solution.
thecalcc
left a comment
thecalcc
left a comment
There was a problem hiding this comment.
let me know if you need help with the positioning
|
scripts/core/ui/ui.ts
Outdated
| // Get position of the active input element | ||
| const rect = element[0].getBoundingClientRect(); | ||
|
|
||
| // Measure popup size (use defaults if not available) |
There was a problem hiding this comment.
why would it not be available? if it's not available why does popup get rendered?
scripts/core/ui/ui.ts
Outdated
| let left = rect.left + scrollX; | ||
|
|
||
| // If not enough space below, open upward | ||
| // If not enough space below, open upward (keep margin so input is not overlapped) |
There was a problem hiding this comment.
Could you make this comment better? "If there isn't enough space available below the input, position the popup above the input"
| if (rect.bottom + popupHeight + TOLERANCE > viewportHeight && rect.top >= popupHeight + GAP + TOLERANCE) { | ||
| top = rect.top + scrollY - popupHeight - GAP; | ||
| } else if (rect.bottom + popupHeight + TOLERANCE > viewportHeight) { | ||
| // Neither side fits fully: clamp into viewport and prefer below. | ||
| top = Math.min(Math.max(scrollY + TOLERANCE, rect.bottom + scrollY + GAP), scrollY + viewportHeight - popupHeight - TOLERANCE); | ||
| } |
There was a problem hiding this comment.
could you make the code here readable?
| // We also re-run the positioning on the next frame and after a small | ||
| const MARGIN = 8; | ||
| const RECHECK_DELAY = 50; | ||
| if (!value) { |
There was a problem hiding this comment.
nitpick - could you have this as an explicit check like value == null
| const viewportWidth = window.innerWidth; | ||
| const scrollX = window.scrollX || window.pageXOffset; | ||
| const scrollY = window.scrollY || window.pageYOffset; | ||
| // Measure popup size (use sensible defaults if not available) |
There was a problem hiding this comment.
in my understanding, if the popup element is not available, this function shouldn't be called. if so, then we don't need fallback values. But if it's still running when popupEl is null, we should fix that first
| const inputHeight = rect.height || (element.outerHeight && element.outerHeight()) || 32; | ||
| const GAP = Math.max(8, Math.round(inputHeight)); |
There was a problem hiding this comment.
why do we need this? and not just rect.height
| } | ||
| // Dynamic gap based on input height so popup doesn't cover the input. | ||
| const inputHeight = rect.height || (element.outerHeight && element.outerHeight()) || 32; | ||
| const GAP = Math.max(8, Math.round(inputHeight)); |
There was a problem hiding this comment.
this is not a gap, it's input height
|
Closing in favour of #4994 |
3 participants
SDBELGA-995
Purpose:
Rework the datepicker popover, so it is positioned correctly, and it's content isn't cut off at the bottom.