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

Create DatePickerInline #33

Closed
wants to merge 1 commit into from
Closed

Create DatePickerInline #33

wants to merge 1 commit into from

Conversation

patagoniapy
Copy link

This PR is a feature submission in reference to #32 . It creates a DatePickeInline component. While it would be more streamlined to create an inline prop on the DatePicker component, this would seemingly require quite a bit of work to refactor the architecture already in place surrounding the Popover.

This has not been fully tested and I'd appreciate any help to make sure all the props are still available on DatePickerInline

@antony
Copy link
Member

antony commented Apr 1, 2021

Hey. This looks good - I'll give it a test as soon as I can. The builds are failing due to quite a lot of lint issues, I'm not sure why but it might be worth looking at that.

@antony
Copy link
Member

antony commented Apr 1, 2021

So, just looking at the linting issues, it looks like you've re-formatted a large part of the project to a different code style - which makes it impossible to review but also I can't merge it unless it matches the project's code style.

I also noticed that the DatePickerInline compoent is essentially the main DatePicker component copy pasted with a few changes. This will immediately double the maintenance workload of the project and risk us fixing issues in one component or not another.

Ideally the "inline" mechanism will simply be displaying the popover component as a block level element in the page, and setting isOpen to true, permanently. All of the other code can be shared.

@patagoniapy patagoniapy closed this Apr 1, 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.

2 participants