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

Fz/date range input footer element #5538

Closed

Conversation

fnziman
Copy link
Contributor

@fnziman fnziman commented Sep 13, 2022

Fixes #5537

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Adds the footerElement to the DateRangePicker

Reviewers should focus on:

Seeing that the footer now works

Screenshot

image

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @fnziman! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

{this.renderCalendars(isShowingOneMonth)}
{this.maybeRenderTimePickers()}
<div className={DateClasses.DATEPICKER_CONTENT}>
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem like the ideal DOM structure... it probably makes more sense for the calendar(s) and time picker(s) to live adjacent to the footer element. I would like to see more options added to the docs example so we can test out this new layout. I'll try to push my ideas as a commit on your branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made an update that I think is more what you are looking for..

<div>
{this.renderCalendars(isShowingOneMonth)}
{this.maybeRenderTimePickers()}
<div className={DateClasses.DATEPICKER_CONTENT}>
Copy link
Contributor

Choose a reason for hiding this comment

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

also this CSS class was named for and designed for "datepicker", not "date range picker", so we'll need to make a new class to keep the naming semantics clear

@adidahiya
Copy link
Contributor

adidahiya commented Sep 13, 2022

Rendering the footer element exposed a few layout issues with the multiple calendars and time pickers rendered by DateRangePicker. I also discovered an unrelated bug where it renders two time pickers even when singleMonthOnly={true}. The fix for all these things was not trivial, so I took your first commit and added my changes on it in a new PR: #5539

@adidahiya
Copy link
Contributor

Thanks for the attempted PR -- I'm going to close this in favor of my new one, but you'll still get credited as a collaborator on the commit when that PR merges :)

@adidahiya adidahiya closed this Sep 13, 2022
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.

[DateRangeInput2]: footerElement prop does not render
3 participants