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 today and clear buttons for inline pickers #782

Conversation

real-marshal
Copy link

This PR closes #

Description

  1. Adds today and clear buttons for inline date, time and datetime pickers.
  2. Allows autoOk prop to affect today button behavior. Since it is true by default for inline pickers, click on today button automatically selects the value and closes the picker.

@codecov
Copy link

codecov bot commented Nov 26, 2018

Codecov Report

❗ No coverage uploaded for pull request base (develop@9f77ae9). Click here to learn what that means.
The diff coverage is 68%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #782   +/-   ##
==========================================
  Coverage           ?   91.07%           
==========================================
  Files              ?       41           
  Lines              ?     1434           
  Branches           ?      173           
==========================================
  Hits               ?     1306           
  Misses             ?       93           
  Partials           ?       35
Impacted Files Coverage Δ
lib/src/DateTimePicker/DateTimePickerInline.tsx 100% <100%> (ø)
lib/src/_shared/BasePicker.tsx 85.1% <100%> (ø)
lib/src/DatePicker/DatePickerInline.tsx 100% <100%> (ø)
lib/src/wrappers/InlineWrapper.tsx 77.33% <60%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f77ae9...ec93d01. Read the comment docs.

@dmtrKovalenko
Copy link
Member

I am sorry. But we are not going to support dialog actions for inline pickers.

@real-marshal
Copy link
Author

real-marshal commented Nov 28, 2018

@dmtrKovalenko Why not? This is rather disappointing, it is important functionality and it should work with inline pickers too. Documentation doesn't even mention that 'showTodayButton' works only with standard pickers.

@stsiarzhanau
Copy link

@dmtrKovalenko Thanks for this awsome pickers. But I have the same question. What are the reasons to not support this functionality? Having ability to clear value in inline datepicker is extremely helpful. So it'd be great to have either "X" button next to calendar icon in date input itself (like in react-datepicker) or "Clear" button in calendar popover itself.

@dmtrKovalenko
Copy link
Member

@sztrzask @RealMarshal dialog actions in the popover is not "material design way" popups must represent 1 action and close. Make "x" adornment to the input similar to keyboard is an option here. But we will not use dialog action with inline pickers

@stsiarzhanau
Copy link

@dmtrKovalenko Thanks for the answer. Then, if it doesn't contradict Material Design principles, may I create an issue (feature request) to add "x" adornment to the input, that will clear selected date?

@dmtrKovalenko
Copy link
Member

@stsiarzhanau of course that will be nice 👍

@kg-currenxie
Copy link

kg-currenxie commented Apr 1, 2019

@dmtrKovalenko you know material design is only recommendations right?
That's why they are called guidelines, not rules.

Dialogs should contain a maximum of two actions.

...
Providing a third action such as “Learn more” is not recommended as it navigates the user away from the dialog, leaving the dialog task unfinished.

There are some cases where an adornment is not the best option either. A general-purpose-community-component like this should not have these kind of restrictions imo.

Modal/full screen date pickers.. never seen. Terrible user experience.
Imagine a row of filters. Most of them would be text fields, drop downs, etc, and suddenly this big massive thing takes over your screen. It's not very consistent in terms of UX.

@zefj zefj mentioned this pull request Nov 6, 2019
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.

4 participants