Calendar component - Calculate and rendering calendar date elements #822

dilandoogan · 2024-04-03T12:49:24Z

It calculates days in the selected year and month and shows in calendar

v3.0.0

AykutSarac · 2024-04-03T12:52:09Z

src/components/calendar/bl-calendar.ts

+import style from "./bl-calendar.css";
+import { TCalendarTypes, TMonth } from "./bl-calendar.types";
+
+@customElement("bl-calendar")


Can we add JSDoc to improve documentation?

AykutSarac · 2024-04-03T12:53:28Z

src/components/calendar/bl-calendar.ts

+    return [style];
+  }
+  render() {
+    this.getWeekDayOfDate();


Do we need this here?

AykutSarac · 2024-04-03T12:55:49Z

src/components/calendar/bl-calendar.ts

+          ${Array.from(calendarDays).map(([key, value]) => {
+            return html`<div class="calendar-column">
+              <div class="weekday-text calendar-cell">${DAYS[key]}</div>
+              ${value.map(day => {
+                return html` <div class="day-text calendar-cell">${day}</div>`;
+              })}
+            </div>`;
+          })}


Can we split it to 2 variables and move out of html tags here?

AykutSarac · 2024-04-03T12:57:07Z

src/components/calendar/bl-calendar.ts

+    let dayOfTheWeek = 0;
+    let iteratedDay = 1;
+
+    for (; i < this.getDayNumInAMonth() + this.getWeekDayOfDate(); i += 1) {


Not sure if we shall place i before the first semicolon here, maybe ensure if it's working fine.

AykutSarac · 2024-04-03T12:59:56Z

src/components/calendar/bl-calendar.types.ts

@@ -0,0 +1,3 @@
+export type TCalendarTypes = "single" | "multiple" | "range";
+export type TMonth = { value: number; name: string };


Can we define strictly what values are valid here? Like value: 1 | 2, and name: "January" etc.

AykutSarac · 2024-04-03T13:01:27Z

src/components/calendar/bl-calendar.types.ts

@@ -0,0 +1,3 @@
+export type TCalendarTypes = "single" | "multiple" | "range";
+export type TMonth = { value: number; name: string };
+export type TCalendarMonths = TMonth[];


We can use TMonth[] at constants/MONTHS instead, if we are not using it anywhere else directly.

erbilnas · 2024-04-03T12:54:11Z

playground/template.html

@@ -33,5 +33,6 @@ <h1>Baklava Playground</h1>
    </p>

    <bl-button>Baklava is ready</bl-button>
+    <bl-calendar></bl-calendar>


We should keep the playground template clean

erbilnas · 2024-04-03T12:56:03Z

src/components/calendar/bl-calendar.constant.ts

@@ -0,0 +1,18 @@
+import { TCalendarMonths } from "./bl-calendar.types";


Instead of using "T," we should use more meaningful names like "Months". It's already imported from types, so we don't need to add "calendar", "types" or anything else.

erbilnas · 2024-04-03T12:58:27Z

src/components/calendar/bl-calendar.constant.ts

+  { name: "December", value: 11 },
+];
+
+export const DAYS = ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"];


Adding types for days as well would be a good idea

erbilnas · 2024-04-03T13:00:59Z

src/components/calendar/bl-calendar.ts

+  static get styles(): CSSResultGroup {
+    return [style];
+  }
+  render() {


Suggested change

render() {

render(): TemplateResult {

Additionally, moving the render method to the bottom of the code would increase readability

erbilnas · 2024-04-03T13:03:15Z

src/components/calendar/bl-calendar.ts

+
+  createCalendarDays() {
+    const monthlyCalendar: Map<number, (number | string)[]> = new Map();
+    let i = 0;


Using meaningful variable names is preferable to using "i"

erbilnas · 2024-04-03T13:08:10Z

src/components/calendar/bl-calendar.ts

+    this.month = MONTHS[monthIndex];
+  }
+
+  setYear(year: number) {


We don't need this setter function imho

why? do you suggest to set like this.year= this.year-1 in where it is used ?

erbilnas · 2024-04-03T13:09:26Z

src/components/calendar/bl-calendar.ts

+  @property({ type: Array })
+  disabledDates?: Date | Date[];
+
+  @state()


Defining states with "_" like "_day" is the most common practice in Lit

erbilnas · 2024-04-03T13:11:12Z

src/components/calendar/bl-calendar.ts

+  }
+
+  setPreviousMonth() {
+    if (this.month.value === 0) {


We can use our MONTHS constant instead of numbers. It would be more readable

erbilnas · 2024-04-03T13:11:18Z

src/components/calendar/bl-calendar.ts

+    } else this.setMonth(this.month.value - 1);
+  }
+  setNextMonth() {
+    if (this.month.value === 11) {


erbilnas · 2024-04-03T13:13:37Z

src/components/calendar/bl-calendar.ts

+          ></bl-button>
+        </div>
+        <div class="weekdays">
+          ${Array.from(calendarDays).map(([key, value]) => {


Why would we need the Array prototype in this context?

erbilnas · 2024-04-03T13:21:23Z

Also, please be mindful of our commit rules: https://baklava.design/?path=/docs/documentation-contributing-baklava-commit-rules--documentation

ozkersemih · 2024-04-03T14:38:33Z

src/components/calendar/bl-calendar.ts

+  type: TCalendarTypes = "single";
+
+  @property()
+  minDate?: Date | string;


I think we should add descriptions for properties like below:

/** *Sets the minimum date value for the calendar */ @property() minDate?: Date | string;

ozkersemih · 2024-04-03T14:40:28Z

src/components/calendar/bl-calendar.ts

+  @property({ type: Array })
+  disabledDates?: Date | Date[];


do we need to describe attribute @property({ type: Array , attribute: "disabled-dates"}) ?

ozkersemih · 2024-04-03T14:49:36Z

src/components/calendar/bl-calendar.ts

+          ${Array.from(calendarDays).map(([key, value]) => {
+            return html`<div class="calendar-column">
+              <div class="weekday-text calendar-cell">${DAYS[key]}</div>
+              ${value.map(day => {
+                return html` <div class="day-text calendar-cell">${day}</div>`;
+              })}
+            </div>`;
+          })}


maybe we can call this part from helper function

Merge pull request #783 from Trendyol/next

c49b425

v3.0.0

AykutSarac reviewed Apr 3, 2024

View reviewed changes

erbilnas requested changes Apr 3, 2024

View reviewed changes

ozkersemih reviewed Apr 3, 2024

View reviewed changes

ozkersemih reviewed Apr 4, 2024

View reviewed changes

dilandoogan added 2 commits April 4, 2024 15:29

calendar component calculate calendar days and render

2d7b05e

changing names to be understandable

3164c27

Enes5519 force-pushed the calendar-component branch from e4dc2d0 to 3164c27 Compare April 4, 2024 12:30

dilandoogan closed this Apr 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calendar component - Calculate and rendering calendar date elements #822

Calendar component - Calculate and rendering calendar date elements #822

dilandoogan commented Apr 3, 2024

AykutSarac Apr 3, 2024

AykutSarac Apr 3, 2024

AykutSarac Apr 3, 2024

AykutSarac Apr 3, 2024

AykutSarac Apr 3, 2024

AykutSarac Apr 3, 2024

erbilnas Apr 3, 2024

erbilnas Apr 3, 2024

erbilnas Apr 3, 2024

erbilnas Apr 3, 2024

erbilnas Apr 3, 2024

erbilnas Apr 3, 2024

erbilnas Apr 3, 2024

dilandoogan Apr 4, 2024

erbilnas Apr 3, 2024

erbilnas Apr 3, 2024

erbilnas Apr 3, 2024

erbilnas Apr 3, 2024

erbilnas commented Apr 3, 2024

ozkersemih Apr 3, 2024

ozkersemih Apr 3, 2024

ozkersemih Apr 3, 2024

		@@ -0,0 +1,3 @@
		export type TCalendarTypes = "single" \| "multiple" \| "range";
		export type TMonth = { value: number; name: string };

		@@ -0,0 +1,18 @@
		import { TCalendarMonths } from "./bl-calendar.types";

Calendar component - Calculate and rendering calendar date elements #822

Calendar component - Calculate and rendering calendar date elements #822

Conversation

dilandoogan commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erbilnas commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment