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

Improve Date/DateTime type parameters #2695

Merged
merged 2 commits into from
Jul 25, 2018
Merged

Improve Date/DateTime type parameters #2695

merged 2 commits into from
Jul 25, 2018

Conversation

kravets-levko
Copy link
Collaborator

Issue #2642

Default date/time inputs replaced with Ant Date/Time picker.

Screenshots:

Date:
image
image

Date/Time:
image
image
image

Date/Time with seconds (same as Date/Time but different format):
image

@vercel
Copy link

vercel bot commented Jul 25, 2018

Since this Pull Request originated from a forked repository, Now cannot deploy it as there are potential security risks.

If you are a collaborator on this repository, consider making this Pull Request from a branch on the same repository instead of a fork.

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

Looks good and much less code than I expected this to be 😅

See some comments.

Also try to "stress" test it with a few edge case. I remember the browser default input field had issues where it would show a value but actually have a null value.

// (react does not support two-way binding with `ngModel`)
this.updateValue = (function updateValue(value) {
this.ngModel = value;
}).bind(this);
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the following?

this.updateValue = value => this.ngModel = value;

(=> instead of .bind(this))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, missed it, you're right


DateInput.defaultProps = {
value: Date.now(),
placeholder: 'Select date',
Copy link
Member

Choose a reason for hiding this comment

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

Select Date

Copy link
Member

Choose a reason for hiding this comment

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

Also seems that we don't really use it customer placeholders. Maybe just pass a static value?

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

One more question: how does it look side by side with regular input fields?

@kravets-levko
Copy link
Collaborator Author

@arikfr Updated PR.

how does it look side by side with regular input fields?

A bit different. I'm not sure - should I fix the CSS, or it's better if @kocsmy will do it? WDYT?
image

@kravets-levko
Copy link
Collaborator Author

Also try to "stress" test it with a few edge case. I remember the browser default input field had issues where it would show a value but actually have a null value.

I tried to use it from users perspective - and it works as it should: it displays value if there is value set, and it shows placeholder when param does not have value set.

@arikfr
Copy link
Member

arikfr commented Jul 25, 2018

A bit different. I'm not sure - should I fix the CSS, or it's better if @kocsmy will do it? WDYT?

There are 3 options here:

  1. Change the Ant component to look like ours (or at least align properly).
  2. Change ours to look like Ant.
  3. Use Ant's Input component.

@kocsmy what are your thoughts?

@kocsmy
Copy link
Collaborator

kocsmy commented Jul 25, 2018

Hmm, it should be fairly simple to change their design to look like ours but if we plan to use more of Ant components or even use their whole library, then that's a bigger plan :)

@arikfr arikfr merged commit d70bcfd into getredash:master Jul 25, 2018
@arikfr
Copy link
Member

arikfr commented Jul 25, 2018

Merged 🤩

@arikfr
Copy link
Member

arikfr commented Jul 25, 2018

@kocsmy

Hmm, it should be fairly simple to change their design to look like ours but if we plan to use more of Ant components or even use their whole library, then that's a bigger plan :)

We will use more and more of their components to the point where we can say that we use the whole lib :)

I merged this already so give this a look and see what's the simplest thing we can do to make it look less alien, before we commit to a bigger plan.

@arikfr arikfr mentioned this pull request Jul 26, 2018
3 tasks
@kocsmy
Copy link
Collaborator

kocsmy commented Jul 26, 2018

Sure, I'll take a look and see if we can improve this quickly.

@kravets-levko kravets-levko deleted the feature/better-datetime-inputs branch February 11, 2019 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants