-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Centre all notification popups #6271
Conversation
b4af33b
to
08ea819
Compare
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.
One small easy change!
const width = 360 | ||
|
||
const SCREEN_WIDTH = window.screen.width | ||
const SCREEN_HEIGHT = window.screen.height |
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.
Since a screen's resolution can be adjusted at run-time, we should maybe re-compute this at popup creation time.
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.
Good idea! I'll fix this right up.
e8db224
to
071c924
Compare
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.
Awesome. I'm super embarrassed that we hadn't realized this was possible yet!
071c924
to
3cdcb8d
Compare
3cdcb8d
to
5bf8a36
Compare
@danfinlay I had to rebase this on the latest |
cc @cjeria |
Closes #6269
This PR centres all the notification popups based on the screen size.
The support for
window.screen.height
andwindow.screen.width
seem good, according to MDN.[1]