[#16377] feat: add calendar to quo2

[mohsen-ghafouri]

Fixes #16377

Summary

This PR add the calendar component to quo2

Light Mode

Dark Mode

Platforms

• Android
• iOS

Steps to test

• Open Status

• Navigate to Quo2 Preview > calendar > calendar

status: ready

[ghost]

Hey @mohsen-ghafouri, and thank you so much for making your first pull request in status-mobile! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2

[siddarthkay]

Hi @mohsen-ghafouri : Thanks a lot for your contribution! Could you please rebase your branch with latest develop to resolve conflicts.

[J-Son89]

please adjust all uses of rn/text to use quo2 markdown.text component instead 👍

[J-Son89]

Great work @mohsen-ghafouri !

[ilmotta]

@mohsen-ghafouri, thank you for working on this PR. I can see you put a lot of effort into it. The code mostly LGTM and I can see you took good care of trying to follow the project guidelines.

Although the PR is basically all green code, the diff is too large for the standards of this repository and it would have been better if you opened at least 2 PRs to solve the original GH issue.

I think there are some important things to fix, that's why I'm marking my review as Request changes.

Great work overall! 💯

[J-Son89]

@ilmotta - some very valid points here. I can answer a few concerns on behalf of @mohsen-ghafouri. He created 1 pr as this is part of the interview task so in some sense it is easier to keep it all together.

In terms of adding more unit tests, I 100% agree however perhaps we can wait for this for now (unless it is a small addition) as this is an opportunity for @mohsen-ghafouri to display his technical capabilities (among other things) so to not get too sidetracked perhaps we can leave that as a follow up?

[ilmotta]

@ilmotta - some very valid points here. I can answer a few concerns on behalf of @mohsen-ghafouri. He created 1 pr as this is part of the interview task so in some sense it is easier to keep it all together.

@J-Son89, @mohsen-ghafouri I imagined that would be the case. It's understandble, but I still think we should not put more burden on the review processes with exceptions for the hiring process. This is a fairly large PR that required a lot of time to review and lots of comments. For core devs, we reject this practice whenever we can, although exceptions do happen every now and then.

It's obviously out of scope to have the discussion about the hiring process here, but I'll just say that I think our team can come up with better ways to assess candidates skills without requiring them to work on large implementations. I know other Clojure (or not) devs that would not apply to a process where they had to write this much code, no way.

In terms of adding more unit tests, I 100% agree however perhaps we can wait for this for now (unless it is a small addition) as this is an opportunity for @mohsen-ghafouri to display his technical capabilities (among other things) so to not get too sidetracked perhaps we can leave that as a follow up?

I think @mohsen-ghafouri already wrote the unit tests, so we're good now. I would be very hesitant to approve the PR given the amount of code dealing with time without tests, and because we know nobody would want to write them later. Perhaps it's time we update our guidelines with suggestions about what should and should not be tested. We all agree quality must be built in, with or without the hiring process on top. And very well said @J-Son89, test code is a good way to assess skills.

[ilmotta]

@mohsen-ghafouri I'll give another review pass today, but it's looking great :)

[ilmotta]

Hey @mohsen-ghafouri, thank you for diligently working on the PR feedback. I think you did a really nice work here!

I left a few other comments and it would be great if you could work on some of them, but you definitely have my approval :)

🚀 🚀

[siddarthkay]

Great first time contribution to the codebase!
Thanks for the hard work @mohsen-ghafouri

[siddarthkay]

@mohsen-ghafouri : now would be a good time to do the following checks :

• make lint
• make lint-fix
• make component-test

If all of them succeed let me know otherwise you may have some minor fix ups to do :)

[J-Son89]

• make lint
• make lint-fix
• make component-test

@mohsen-ghafouri once you have addressed these checks that @siddarthkay mentioned, feel free to ping Francesca-G for a design review :)

[mohsen-ghafouri]

I wanted to express my gratitude for all your valuable feedback and comments on the PR. Your input has been immensely helpful in improving the code. @ilmotta @siddarthkay @J-Son89 @flexsurfer

the PR is now ready for design review @Francesca-G

[Francesca-G]

here's the Figma frame with the design review.
Just a minor detail, great work :)

[mohsen-ghafouri]

@Francesca-G can you give me access to see the file? maxghfori@gmail.com

[mohsen-ghafouri]

@Francesca-G Sorry my bad, wrong email address 😓. please give access to this email maxghafori@gmail.com

[J-Son89]

@mohsen-ghafouri @Francesca-G as I see, the only issue in the design review is actually a bug in the outline variant of the button -I will address that in: #16772

[siddarthkay]

So should we import this to see if CI is green? @J-Son89

[J-Son89]

So should we import this to see if CI is green? @J-Son89

Yep can be merged afaik!

[siddarthkay]

Once this is green : https://ci.status.im/blue/organizations/jenkins/status-mobile%2Fprs%2Ftests/detail/PR-16809/1/pipeline we can rebase and merge 👍🏻

[siddarthkay]

Tests are green, we can update branch and merge!