Basic/empty calendar - [merged] #42

Closed thisismydesign closed 4 years ago

In GitLab by @gomorizsolt on Mar 20, 2019, 09:09

_Merges issue-7basic-calendar -> master

Issue #9.

Created a static JSON calendar to prepare for the SVG displaying.

Questions:

The data-date property within each children is fixed. Should we update them based on the current date? The other case is that it doesn't really matter since we'll only display the colors in the beginning.
How about increasing the size of the SVG along with the tiny squares? We can even leave some more gaps among them.
Responsiveness? This might be a future discussion.

Any ideas about the appearance of the SVG are welcome. Attached a screenshot below.

Képernyőfotó_2019-03-20_-_9.00.32

In GitLab by @gomorizsolt on Mar 20, 2019, 09:12

changed the description

In GitLab by @thisismydesign on Mar 20, 2019, 11:01

Responsiveness

Let's not worry about it for now.

dates

Is this necessary for the merge function? The month values should be correct so I guess it is? In that case, there should be a function which can take this and make it into an "up-to-date" empty calendar.

In GitLab by @thisismydesign on Mar 20, 2019, 11:04

Size / visibility.

Necessary requirements:

texts are spaced correctly, look much alike to GH (afaict this mostly means using a smaller font)
texts do not cover the calendar

Nice to have requirements:

looks almost perfectly matching GH
hover text over the squares

Feel free to extract this into an issue if needed, sicne they're not all that related to this PR.

In GitLab by @gomorizsolt on Mar 20, 2019, 11:12

Yes, the month values should be correct in any case. I'll try to accomplish a function that handles the "up-to-date" functionality.

Regarding the appearance, as you proposed, I'll create a separated issue to make it clear.

In GitLab by @gomorizsolt on Mar 20, 2019, 11:18

changed title from Basic{- calendar - JSON-} to Basic{+/empty calendar+}

In GitLab by @gomorizsolt on Mar 20, 2019, 11:29

changed the description

In GitLab by @gomorizsolt on Mar 20, 2019, 12:55

marked as a Work In Progress

In GitLab by @gomorizsolt on Mar 20, 2019, 14:05

added 1 commit

eef3de38 - Add GetUpdatedBasicCalendar

Compare with previous version

In GitLab by @gomorizsolt on Mar 20, 2019, 14:05

Implemented the feature of updating the contribution dates to the corresponding ones. I'd appreciate if you also tried the functionality out locally.

I'll create the missing tests later.

In GitLab by @gomorizsolt on Mar 21, 2019, 08:49

added 1 commit

0c9e809e - Refine GetUpdatedBasicCalendar, add immunate updating

Compare with previous version

In GitLab by @gomorizsolt on Mar 21, 2019, 09:09

added 2 commits

d0529ca1 - Add tests to GetUpdatedBasicCalendar
0cb4fcec - Change the order of variables in GetUpdatedBasicCalendar

Compare with previous version

In GitLab by @gomorizsolt on Mar 21, 2019, 09:15

Missing tests have been completed and I also refined the function to update the basic calendar object properly. Otherwise, here are some questions:

Is that the appropriate way to test functions like this?
How about creating a CalendarUtils folder? We may create other utility functions later that are related to the (basic) calendar.
I couldn't figure out more expressive function names. If they're not satisfying, could you propose a few examples?

In GitLab by @gomorizsolt on Mar 21, 2019, 09:19

unmarked as a Work In Progress

In GitLab by @gomorizsolt on Mar 21, 2019, 09:21

added 1 commit

fd019aa1 - Rename GetUpdatedBasicCalendar to GetUpToDateCalendar

Compare with previous version

In GitLab by @gomorizsolt on Mar 21, 2019, 09:51

added 1 commit

843043fe - Fix typo in GetUpToDateCalendar.test.js

Compare with previous version

In GitLab by @thisismydesign on Mar 21, 2019, 10:13

Why is string formatting needed there?

new Date(new Date().setFullYear(new Date().getFullYear() - 1)).toLocaleDateString() == new Date(new Date().setFullYear(new Date().getFullYear() - 1)).toLocaleDateString().replace(/. /g, '-').slice(0, 10);
> true

In any case, date formatting should not be done with string formatting. I'm sure there are libraries for that and toLocaleDateString also takes format params: https://stackoverflow.com/a/34015511/2771889

In GitLab by @thisismydesign on Mar 21, 2019, 10:13

Same date formatting issue here

In GitLab by @thisismydesign on Mar 21, 2019, 10:14

Same date formatting issue here

In GitLab by @thisismydesign on Mar 21, 2019, 10:16

I'd also make sure that the last date is correct. Those two should make us fairly certain that it works.

In GitLab by @thisismydesign on Mar 21, 2019, 10:17

GetTodaysCalendar maybe?

In GitLab by @thisismydesign on Mar 21, 2019, 10:18

Left a few comments, also agree with CalendarUtils, you can just call the file that too.

In GitLab by @gomorizsolt on Mar 21, 2019, 10:18

Sounds much better, thanks.

In GitLab by @gomorizsolt on Mar 21, 2019, 10:26

That's a good idea, thanks.

In GitLab by @gomorizsolt on Mar 21, 2019, 10:27

Does it mean that it's not a good practice to rely on string transformer functions in the case of dates? For example replace, slice and so forth.

Btw you're absolutely right, this should be handled somehow in JS.

In GitLab by @thisismydesign on Mar 21, 2019, 10:31

Yep. There're usually very extensive libraries to do this for you. I can show an example from ruby: https://apidock.com/ruby/DateTime/strftime

In GitLab by @thisismydesign on Mar 21, 2019, 10:32

But here it also seems to me that no formatting is required, hence the code example.

In GitLab by @gomorizsolt on Mar 21, 2019, 10:41

I don't really get it. Why don't we need to format the date?

In GitLab by @thisismydesign on Mar 21, 2019, 10:42

Because

new Date(new Date().setFullYear(new Date().getFullYear() - 1)).toLocaleDateString() == new Date(new Date().setFullYear(new Date().getFullYear() - 1)).toLocaleDateString().replace(/. /g, '-').slice(0, 10);
> true