Fixes #1483, #1484, #1482 - Clay Datepicker and Clay Timepicker

[matuzalemsteles]

hey guys, from the rewrite of this component in React, we can withdraw from here the Button, Select, Icon and Dropdown components of Datepicker but I still do not know how we will proceed with rewriting the existing components.

[matuzalemsteles]

Just a little extra information about the missing tests here: Since we are experimenting with hooks (not yet stable) and implementing the new components in React, we would need to prepare the tests infrastructure to be able to use enzyme for example, so a temporary solution would be to configure the tests in each package, but hooks still do not support shallow rendering. I'm following facebook/react#14091.

[jbalsas]

/cc @julien, @wincent, @bryceosterhaus, @carloslancha, care to put some 👀 here?

[wincent]

Seems this clay-datepicker should be clay-date-picker (and ClayDatePicker) for consistency with clay-color-picker (#1427) and what seems to be the pattern in the existing components.

[bryceosterhaus]

Looks good so far!

Out of curiosity, how did you like working with hooks for this?

[matuzalemsteles]

hey @bryceosterhaus about my experience with Hooks: I liked hooks mainly because of the idea of ​​being able to reuse logic this is indisputably essential (In the components in Clay for example, using class we were already starting to repeat many logics in several components, hooks will help enough to remove this...), I think I have not used all the power that the hooks have to give better feedback on, but I think we're figuring out how it's fitting...

[wincent]

hey @bryceosterhaus about my experience with Hooks: I liked hooks mainly because of the idea of ​​being able to reuse logic this is indisputably essential (In the components in Clay for example, using class we were already starting to repeat many logics in several components, hooks will help enough to remove this...),

Will be interesting to see how this plays out @matuzalemsteles, because I agree with you that the primary selling point of hooks is extracting reusable logic into shareable, testable units. But having said that, there are many times when we architect things for re-use and then we never actually re-use them 😂 — and it is very easy to fall into the mindset of "when you have a hammer, everything looks like a nail". Having said that though, if we started using hooks pervasively, even when re-use is not a motivation, we might still get some benefit from the simplicity of doing something consistently, always using a single pattern. But I don't know yet, so I'll be interested to see where things settle.

[matuzalemsteles]

hey @wincent yes, this is a concern. I see some places that the hooks can help us here, such as Dropdown, we have the logic of keyboard navigation in Dropdown but we have another one for MultiSelect to cover the use cases of there... we can have with hooks only the navigation logic for example.

I think we have fairly strong reuse of components here whenever possible, as Lexicon uses the Atomic Design approach fortifies that too... I think hooks will help to fortify logic reuse of components (I hope...) but I speak at the point of view over non-public APIs.

But I'm not just basing on hooks just in that thought, I think hooks have a peculiar thing about you doing small micro optimizations (I know that while having this possibility it can make things more complex too...) that in the end are always welcome.

In my point of view, I think going with class would be using a lot of HOCs that although it is an interesting approach, it creates a lot of classes and maybe a giant tree of wrappers... the functions covers that side but at the same time we will have many functions... I think we will see how this will fit better with time and until there continue to maintain organization and consistency.

[matuzalemsteles]

hey @pat270, I ended up extracting the CSS that I did in the demo to ClayCSS, when you can have some time, can you look at this? Thanks!

[matuzalemsteles]

hey guys, one person raised a concern for me today about ClayTimePicker using the native type="time" browser only, this is not supported in Safari and IE, so we would have to support an alternate time for those browsers.

https://caniuse.com/#feat=input-datetime

cc @victorvalle @jbalsas

[jbalsas]

Hey @matuzalemsteles, the idea is to only use that on mobile browsers. IOS Safari actually supports it and offers a way better UX than our component, so this should be fine. We will never use the native input in Desktop devices.

[matuzalemsteles]

hey @bryceosterhaus, I ended up removing defaultProps from hooks functions, were not working well with typescript, has a issue related to React types DefinitelyTyped/DefinitelyTyped#30695 to solve this but it seems that React has an RFC that will depreciate the defaultProps for functions but nothing merged yet (https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#deprecate-defaultprops-on-function-components).

[matuzalemsteles]

hey guys, I think now it's for a final review, we have the markups and CSS ready and I've rewritten for typescript and standardization according to what was defined in the generator.

[wincent]

I really like the TypeScript version of this! I left some comments inline about the JS, but I'd be fine with merging this and iterating on it there as opposed to having a Never-Ending PR™.

There is a lot going on in the SCSS — I'm a bit surprised the diff is so big — but I'm not very familiar with our patterns there yet so I didn't leave any inline comments.

There are a few lengthy values like rgba(lighten(saturate(adjust-hue($secondary, -2), 5.48), 37.06), 0.5) that get repeated in a few places which might be nice to DRY up into named variables instead.

I also see that we have this pre-existing pattern of calling map-get a zillion times to assign a value to a variable that is probably used only once; do you know why we don't just do it inline?

// inline:
height: map-get($map, height);

// as opposed to:
$height: map-get($map, height);

// followed many lines later by:
height: $height;

[matuzalemsteles]

hey @wincent, about CSS I do not know what motives it will take @pat270 to use this way, maybe it will respond better than I do. Recently it is changing some things...

[pat270]

I also see that we have this pre-existing pattern of calling map-get a zillion times to assign a value to a variable that is probably used only once; do you know why we don't just do it inline?

@wincent that is a good point. I ended up using this pattern early on because I did re-use variables in some mixins and wanted to keep the pattern consistent. I started to move away from re-using variables inside mixins because I thought it's too confusing/limiting for devs not familiar with the code. I'm still trying to find the sweet spot between customization and ease of use. I eventually want to document all the code using http://sassdoc.com/?

There are a few lengthy values like rgba(lighten(saturate(adjust-hue($secondary, -2), 5.48), 37.06), 0.5) that get repeated in a few places which might be nice to DRY up into named variables instead.

The Lexicon Team gave me some names for the colors recently. We can change it to that.

Edit: Forgot to mention for the lengthy values, can we merge this and create a separate issue for that? It was already merged in 2.x-develop that way I can update both of them at the same time.

[matuzalemsteles]

hey @pat270 adding SASSDOC would be great, maybe we could add that documentation to clayui.com automatically in some way. It's a great content.

[wincent]

@pat270: thanks for the context!

[matuzalemsteles]

Edit: Forgot to mention for the lengthy values, can we merge this and create a separate issue for that? It was already merged in 2.x-develop that way I can update both of them at the same time.

Can you take care of creating a separate issue? as soon as that merges with branch develop, you can work on it.

hey @wincent and @bryceosterhaus I'm going to go ahead with this PR for branch develop, I believe that from this PR we can get some components that will be created in # 1711, # 1710. If have more changes to this PR I'll take care care of updating, if anyone has any other opinion or change request feel free to comment here.

This PR also contains some CI fixes.

[wincent]

I'm going to go ahead with this PR for branch develop

Sounds like a good idea. GitHub isn't actually very good for code review, and the things that make it not good for code review only get worse the longer a PR lives and the more activity it has. 😭

[matuzalemsteles]

Merged!