-
Notifications
You must be signed in to change notification settings - Fork 355
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
Position date picker dialog below help text #1584
Position date picker dialog below help text #1584
Conversation
Regression test coverage:Examples without any regression tests:
Examples missing some regression tests:
Example pages with Keyboard or Attribute table rows that do not have data-test-ids:
SUMMARY:55 example pages found. ERROR - missing tests: Please write missing tests for this report to pass. |
Thanks, @danieldafoe! |
@carmacleod Ah, that makes sense indeed! I can make that change instead. |
@carmacleod I hope you don't mind, but my design eye found that Let me know what you think! |
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.
Looks great!
Thanks @danieldafoe !
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.
I'm perfectly fine with this change but it doesn't actually solve the issue of the calendar butotn obscuring the help text, in chrome or firefox, as seen in my comment here: #1531
Screen capture is in the issue 1531 |
Per comment from @spectranaut, I'm removing the association between this PR and #1531. Nonetheless, we have consensus that this is an improvement in the dialog positioning. So, going forward with the merge. Thank you @danieldafoe |
Originally intended to address issue #1531. As it turns out, that issue might only be reproduceable on Linux and is slightly different; it is related to the position of the button, not the dialog.
Preview