Skip to content

Updated the dialog positioning to be higher up #4906

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

Merged
merged 2 commits into from
Jun 6, 2022

Conversation

elebitzero
Copy link
Member

@elebitzero elebitzero commented May 31, 2022

I noticed, especially on a high resolution 4k monitor, that the positioning of the Rename Column dialog was different now from OR 3.5.2.

In OR 3.5.2, window.prompt() is used and on Brave/Chrome, the prompt dialog appears right at the top, even overlapping the browser URL bar.

In OR 3.6.0, I changed it to a jquery-ui dialog to match the UX of the rest of the webapp. This looks better, but the positioning is changed. See the screenshots below:

I looked into the positioning code in dialog.js, and found that the code is centering the dialog's midline on the window's midline.

Current UX guidelines suggest modal dialogs should be positioned higher in the window. The current behavior looks especially bad for small dialogs on high resolution 4k monitors. (See screenshot below).

I noticed further incorrect behavior for some of the larger Export dialogs where if your browser window was small enough, the dialog was positioning with a negative top position so that the top of the dialog appeared underneath the browser URL bar.

Changes proposed in this pull request:
I have changed the initial position of dialogs so that:

  • Dialogs are positioned at the top with a margin of 10% of the window height. This gives consistent positioning.
  • If the dialog is so large it is > 90% of the window height, the dialog will be moved further up.
  • A minimum 5px margin between the browser bar and the dialog will be enforced.

This seems to work for both small and large dialogs, and for small window sizes (like on a 1280x768 laptop), and large window sizes (4k monitor)

I also had to change the default Cypress viewport from 1000px x 660px (default) to 1280 x 768, because after the above changes, one test was failing because it relied on the export dialog having a negative position.

OR 3.5.2
image

OR 3.6.0
image

With the fix
image

Copy link
Member

@wetneb wetneb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me! Leaving open longer to let others chime in.

@wetneb wetneb merged commit afeae29 into OpenRefine:master Jun 6, 2022
antoine2711 pushed a commit that referenced this pull request Jun 21, 2022
* Updated the dialog positioning to be higher up and not have negative position (up and off the screen).

* Updated Cypress dialog size.
antoine2711 pushed a commit that referenced this pull request Jun 21, 2022
* Updated the dialog positioning to be higher up and not have negative position (up and off the screen).

* Updated Cypress dialog size.
antoine2711 pushed a commit that referenced this pull request Jun 21, 2022
* Updated the dialog positioning to be higher up and not have negative position (up and off the screen).

* Updated Cypress dialog size.
antoine2711 pushed a commit that referenced this pull request Jun 21, 2022
* Updated the dialog positioning to be higher up and not have negative position (up and off the screen).

* Updated Cypress dialog size.
antoine2711 pushed a commit that referenced this pull request Jun 21, 2022
* Updated the dialog positioning to be higher up and not have negative position (up and off the screen).

* Updated Cypress dialog size.
antoine2711 pushed a commit that referenced this pull request Jun 21, 2022
* Updated the dialog positioning to be higher up and not have negative position (up and off the screen).

* Updated Cypress dialog size.
antoine2711 pushed a commit that referenced this pull request Jul 6, 2022
* Updated the dialog positioning to be higher up and not have negative position (up and off the screen).

* Updated Cypress dialog size.
@elebitzero elebitzero deleted the dialog-position-fix branch October 26, 2022 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants