search

deprecate / metal-clay-components

10 stars 14 forks source link

LEXI-64 ClayModal #125

Closed carloslancha closed 6 years ago

carloslancha commented 6 years ago

Hey @matuzalemsteles ! I've done some changes in the modal to adjust it better to the markup and to get rid of those setTimeouts. What do you think? :)

matuzalemsteles commented 6 years ago

Just started reviewing :)

:octocat: Sent from GH.

matuzalemsteles commented 6 years ago

Hey @carloslancha, Much better now, thanks for the tweaks.

What do you think to create a test to cover the use of show ()?

carloslancha commented 6 years ago

Hey @matuzalemsteles sure, we need to cover that method, but we definitely should not use timeouts on tests, I hadn't time today to work on that but I wanted to send it because I'll be on vacation till next thursday. Could you work on it if you have time?

Thx and good job!

carloslancha commented 6 years ago

Another thing I thought about is that maybe we should have a hide method that calls hide event, and work with show method in the same way we're doing with hide, by emitting a show event and calling a _defaultShowModal or something like that. What do you think @matuzalemsteles ?

matuzalemsteles commented 6 years ago

Hey matuzalemsteles sure, we need to cover that method, but we definitely should not use timeouts on tests, I hadn't time today to work on that but I wanted to send it because I'll be on vacation till next thursday. Could you work on it if you have time?

Okay, I'll take a look at this. By the way, a good vacation.

Another thing I thought about is that maybe we should have a hide method that calls hide event, and work with show method in the same way we're doing with hide, by emitting a show event and calling a _defaultShowModal or something like that. What do you think matuzalemsteles ?

Look good to me, we can put a show event in attached just like hide and do that. I'll work on it. Thanks!

matuzalemsteles commented 6 years ago

Resent #137