Skip to content
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

Add appendTo features of go-datepicker module #720

Merged
merged 8 commits into from
Jan 7, 2021
Merged

Conversation

amitpatra91
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added
  • Docs have been added or updated

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Style Update (CSS)
  • Other... Please describe:

What is the current behavior?

After clicking on the calendar icon calendar body was hiding behind of modal body.

Resolves 717

What is the new behavior?

Now it's displayed on the modal body.

Does this PR introduce a breaking change?

  • Yes
  • No

@amitpatra91 amitpatra91 changed the title Add appendTo features go-datepicker module Add appendTo features of go-datepicker module Nov 26, 2020
Copy link
Contributor

@grahamhency grahamhency left a comment

Choose a reason for hiding this comment

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

This is a good first pass, I think we can make some improvements and simplifications though. Please see my comments.

Also, I looked at the deploy preview for this to see the functionality and there is a bug. To reproduce:

  1. Go to the example in the docs and open the modal
  2. Open the datepicker
  3. Click the close icon for the modal
  4. The datepicker remains on the page

Screenshot:
Screen Shot 2020-12-07 at 12 24 51 PM

Copy link
Contributor

@grahamhency grahamhency left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, had to do some research and some trial & error to figure out how to not use the setTimeout and other things. I've made some more comments, let me know your thoughts.

Copy link
Contributor

@grahamhency grahamhency left a comment

Choose a reason for hiding this comment

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

Just one more comment on types.

Additionally, I think we need to write some tests for these changes and also fix the broken tests.

Copy link
Contributor

@grahamhency grahamhency left a comment

Choose a reason for hiding this comment

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

Code is good, but there are broken tests and we need tests for the updated code.

Copy link
Contributor

@grahamhency grahamhency left a comment

Choose a reason for hiding this comment

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

The changes you proposed for the tests is not a solution. Those tests should still be in place an pass successfully.

@grahamhency grahamhency merged commit 57447e1 into dev Jan 7, 2021
@grahamhency grahamhency deleted the go-datepicker branch January 7, 2021 15:31
@grahamhency grahamhency mentioned this pull request Jan 7, 2021
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.

[BUG] Datepicker calendar hidden inside of modal.
2 participants