Skip to content

improvement: adding a prop called variant for datepicker range#133

Merged
duranmla merged 1 commit intofreenowtech:mainfrom
lloydaf:improvement/introduce-compact-variant-for-daterangepicker
Jun 29, 2021
Merged

improvement: adding a prop called variant for datepicker range#133
duranmla merged 1 commit intofreenowtech:mainfrom
lloydaf:improvement/introduce-compact-variant-for-daterangepicker

Conversation

@lloydaf
Copy link
Contributor

@lloydaf lloydaf commented Jun 28, 2021

What: This PR intends to add a variant property to the datepicker


Why: This is introduced to enable developers to decide whether to show only a single month or two months, according to their needs, without breaking the current responsive behavior.


How:

  1. Create a prop variant


Media: https://www.loom.com/share/9cf8700746d14419a0c4610face30006

Checklist:

  • Ready to be merged

Copy link
Contributor

@duranmla duranmla left a comment

Choose a reason for hiding this comment

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

Love it. It is simple.
Nonetheless, I will create an issue for the future in order to explore https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Container_Queries

@duranmla duranmla merged commit bc635b0 into freenowtech:main Jun 29, 2021
github-actions bot pushed a commit that referenced this pull request Jun 29, 2021
## [1.8.0](v1.7.0...v1.8.0) (2021-06-29)

### Features

* **date-range:** Allow to force picker to show a single month via props ([#133](#133)) ([bc635b0](bc635b0))
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

* @value `'compact'` displays only a single month
* @default 'normal'
*/
variant?: 'compact' | 'normal';
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually variant describes the difference between a bottom-lined and a boxed input (at the moment we only have boxed date picker versions). Have you considered exposing the numberOfMonths field instead?

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 thought about it, but it seemed more technical and on the path of being a workaround rather than a solution. Plus, that opens up the pandoras box of being able to have 3 months displayed, 4 months displayed etc, which will require changes in the component logic to not break anything, imo.

If you are strongly opinionated about this, maybe we can expose something like displaySingleMonth: boolean or other alternatives, but compact seemed much more developer friendly :)

I know the PR is merged already, but happy to keep the conversation going.

ref={ref}
// TODO: refer to https://stash.intapps.it/projects/DS/repos/wave/pull-requests/104/overview?commentId=168382
numberOfMonths={window.innerWidth >= 768 ? 2 : 1}
numberOfMonths={variant === 'normal' && window.innerWidth >= 768 ? 2 : 1}
Copy link
Contributor

Choose a reason for hiding this comment

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

In case we go for this, we should also add a test please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants