-
Notifications
You must be signed in to change notification settings - Fork 296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed schedule rotations form #3904
Conversation
@@ -97,3 +96,21 @@ | |||
margin-bottom: 10px; | |||
width: 100%; | |||
} | |||
|
|||
@media (max-height: 768px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of a must because the scrollable container should scale differently based on window height (e. g. 45vh too less for anything higher than 1024px)
@@ -421,6 +422,13 @@ export const RotationForm = observer((props: RotationFormProps) => { | |||
} | |||
}, [store.timezoneStore.selectedTimezoneOffset]); | |||
|
|||
useEffect(() => { | |||
window.addEventListener('resize', onResize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hide modal on resize so that it's not accidentally moved into edges where it's not visible (react-draggable bounds only work on dragging, not generic placement)
@@ -699,18 +707,38 @@ export const RotationForm = observer((props: RotationFormProps) => { | |||
</> | |||
); | |||
|
|||
function onResize() { | |||
onHide(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not call onHide
directly without creating proxy fn as it does only one thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the passed prop could change reference and you no longer remove the correct associated handler, that was my thinking back then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we can leave like that, I missed it's passed as a prop 👍
# What this PR does - Improves the calculation of dragging area limits - Fixes some minor margin/padding inconsistencies for which horizontal scrollbar was appended ## Which issue(s) this PR fixes #3868 ## Checklist - [ ] Unit, integration, and e2e (if applicable) tests updated - [ ] Documentation added (or `pr:no public docs` PR label added if not required) - [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required)
What this PR does
Which issue(s) this PR fixes
#3868
Checklist
pr:no public docs
PR label added if not required)CHANGELOG.md
updated (orpr:no changelog
PR label added if not required)