Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(date-picker): [date-picker] Adapting to the old-theme #2575

Merged
merged 3 commits into from
Nov 29, 2024
Merged

feat(date-picker): [date-picker] Adapting to the old-theme #2575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
963ab8f
feat(date-picker): [date-picker] Adapt to the theme
Youyou-smiles Nov 22, 2024
17d7e6c
Merge branch 'dev' of github.com:opentiny/tiny-vue into lty/feat-date…
Youyou-smiles Nov 28, 2024
9e7e8b0
feat(date-picker): [date-picker] Adapting to the old-theme
Youyou-smiles Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/theme/src/base/old-theme.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@
--tv-color-bg-active-control: var(--tv-base-color-brand); // #5e7ce0 主色蓝，单选/复选选中背景色、开关开的背景色、选块点击背景色、滑块选中背景色、价格曲线、区域选择选中的背景色
--tv-color-bg-active-primary: var(--tv-base-color-brand); // #5e7ce0 品牌色，锚点-当前位置的圆点背景色/leftmenuthin左侧导航收起图标选中的背景色、NPS评分选中背景色
--tv-color-bg-active-secondary: var(--tv-base-color-common-1); // #adb0b8 次要背景色active色/下拉选中的背景色/一级tab页签背景-选中色/树控件选中背景颜色
--tv-color-bg-active-emphasize: var(--tv-base-color-brand); // #5e7ce0 时间选择选中背景色
--tv-color-bg-active-emphasize: var(--tv-base-color-brand-2); // #5e7ce0 时间选择选中背景色
--tv-color-bg-active-emphasize-light: var(--tv-base-color-brand-1); // #f0f7ff 表格选中背景色
--tv-color-bg-active-dark: var(--tv-base-color-common-3); // #eef0f5 深色背景色激活色

Expand Down
8 changes: 4 additions & 4 deletions packages/theme/src/date-table/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,19 +260,19 @@
border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
}
&__row .in-range:first-of-type div {
&__row td:first-of-type div {
border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
}
&__row .in-range:last-of-type div {
&__row td:last-of-type div {
border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
}
Comment on lines +263 to 270
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Review border radius styling for first/last cells

The current implementation has several concerns:

  1. Using --tv-DateTable-td-border-radius-disabled for all cells might not be appropriate as it's meant for disabled states
  2. The broad selectors td:first-of-type and td:last-of-type will apply border radius to all cells, which might not be desired for middle cells in a range selection

Consider using a more specific approach:

-  &__row td:first-of-type div {
+  &__row td:first-of-type:not(.in-range) div {
     border-top-left-radius: var(--tv-DateTable-td-border-radius);
     border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
   }

-  &__row td:last-of-type div {
+  &__row td:last-of-type:not(.in-range) div {
     border-top-right-radius: var(--tv-DateTable-td-border-radius);
     border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
&__row td:first-of-type div {
border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
}
&__row .in-range:last-of-type div {
&__row td:last-of-type div {
border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
}
&__row td:first-of-type:not(.in-range) div {
border-top-left-radius: var(--tv-DateTable-td-border-radius);
border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
}
&__row td:last-of-type:not(.in-range) div {
border-top-right-radius: var(--tv-DateTable-td-border-radius);
border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
}

&__row td:nth-child(1 of .available.in-range) div {
&__row td.pre-month + td.available.in-range div {
border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
}
&__row td:nth-last-child(1 of .available.in-range) div {
&__row td.available.in-range:has(+ td.next-month) div {
border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
Comment on lines +263 to 277
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

The border radius variable usage is incorrect

The code is using --tv-DateTable-td-border-radius-disabled for non-disabled states, which is inconsistent with the codebase patterns. The search results show that:

  1. The codebase uses --tv-DateTable-td-border-radius for normal states
  2. --tv-DateTable-td-border-radius-disabled is specifically used for disabled states

The code should be modified to use the correct variables:

  &__row td:first-of-type div {
-    border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
+    border-top-left-radius: var(--tv-DateTable-td-border-radius);
+    border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
  }
  &__row td:last-of-type div {
-    border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
+    border-top-right-radius: var(--tv-DateTable-td-border-radius);
+    border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
  }
  &__row td.pre-month + td.available.in-range div {
-    border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
+    border-top-left-radius: var(--tv-DateTable-td-border-radius);
+    border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
  }
  &__row td.available.in-range:has(+ td.next-month) div {
-    border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
+    border-top-right-radius: var(--tv-DateTable-td-border-radius);
+    border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
  }
🔗 Analysis chain

Review the border radius variable usage and browser compatibility

There are several concerns with the new selectors:

  1. The selectors are using --tv-DateTable-td-border-radius-disabled variable for non-disabled states, which seems incorrect. Consider using a more appropriate variable like --tv-DateTable-td-border-radius for normal states.

  2. The :has() selector (line 275) has limited browser support. Consider providing a fallback for better browser compatibility.

Consider applying this refactor for better maintainability and consistency:

-  &__row td:first-of-type div {
-    border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
-  }
-
-  &__row td:last-of-type div {
-    border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
-  }
-
-  &__row td.pre-month + td.available.in-range div {
-    border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
-  }
-
-  &__row td.available.in-range:has(+ td.next-month) div {
-    border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
-  }
+  // Border radius for normal states
+  &__row {
+    td:first-of-type div {
+      border-top-left-radius: var(--tv-DateTable-td-border-radius);
+      border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
+    }
+
+    td:last-of-type div {
+      border-top-right-radius: var(--tv-DateTable-td-border-radius);
+      border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
+    }
+
+    // Range selection borders
+    td.pre-month + td.available.in-range div {
+      border-top-left-radius: var(--tv-DateTable-td-border-radius);
+      border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
+    }
+
+    // Fallback for browsers that don't support :has
+    td.available.in-range.next-month-boundary div {
+      border-top-right-radius: var(--tv-DateTable-td-border-radius);
+      border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
+    }
+
+    // Modern browsers
+    @supports selector(:has(*)) {
+      td.available.in-range:has(+ td.next-month) div {
+        border-top-right-radius: var(--tv-DateTable-td-border-radius);
+        border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
+      }
+    }
+  }

This refactor:

  1. Uses appropriate border radius variables
  2. Provides better organization of selectors
  3. Adds browser compatibility support
  4. Maintains the same functionality while being more maintainable

Let's verify the browser support for the :has selector:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check browser support for :has selector
curl -s "https://caniuse.com/api/features?feature=css-has" | jq '.data."css-has".support'

Length of output: 91

Script:

#!/bin/bash
# Let's check if there are any existing CSS variables defined for border radius
rg "tv-DateTable-td-border-radius" -A 2 -B 2

# Also check for any other border radius variables that might be more appropriate
rg "border-radius.*var\(--" -A 2 -B 2

Length of output: 104115

}
Comment on lines +271 to 278
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Revise range border styling implementation

The verification reveals that:

  1. The :has() selector is widely used across the codebase, and the project's browserslist configuration ("> 1%, last 2 versions") suggests modern browser support is acceptable
  2. The --tv-DateTable-td-border-radius-disabled variable is consistently used for disabled states, while --tv-DateTable-td-border-radius is used for normal states, confirming that using the disabled variant for range cells is incorrect
  3. The suggested alternative using :last-child might miss edge cases, as the current :has() implementation is more precise for handling next-month transitions

Recommended changes:

   &__row td.pre-month + td.available.in-range div {
-    border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
+    border-top-left-radius: var(--tv-DateTable-td-border-radius);
+    border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
   }
   &__row td.available.in-range:has(+ td.next-month) div {
-    border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
+    border-top-right-radius: var(--tv-DateTable-td-border-radius);
+    border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
   }
🔗 Analysis chain

Review range border styling implementation

The current implementation has several concerns:

  1. The :has() selector has limited browser support
  2. Using --tv-DateTable-td-border-radius-disabled for range cells seems incorrect
  3. The current approach might not handle all edge cases correctly

Consider the following improvements:

   &__row td.pre-month + td.available.in-range div {
-    border-top-left-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-left-radius: var(--tv-DateTable-td-border-radius-disabled);
+    border-top-left-radius: var(--tv-DateTable-td-border-radius);
+    border-bottom-left-radius: var(--tv-DateTable-td-border-radius);
   }

-  &__row td.available.in-range:has(+ td.next-month) div {
+  &__row td.available.in-range:last-child div,
+  &__row td.available.in-range + td.next-month div {
-    border-top-right-radius: var(--tv-DateTable-td-border-radius-disabled);
-    border-bottom-right-radius: var(--tv-DateTable-td-border-radius-disabled);
+    border-top-right-radius: var(--tv-DateTable-td-border-radius);
+    border-bottom-right-radius: var(--tv-DateTable-td-border-radius);
   }

Let's verify browser compatibility requirements:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are any browser compatibility requirements in the project
rg -l "browserslist|@supports|\.browserlist" --type yaml --type json

Length of output: 116

Script:

#!/bin/bash
# Check the browser compatibility configuration in package.json files
cat package.json packages/theme-saas/package.json | jq -r '.browserslist // empty'

# Check if :has() selector is already used elsewhere in the codebase
rg -l ":has\(" --type css --type less

# Check for existing border radius variables in theme files
rg "tv-DateTable-td-border-radius" --type css --type less

Length of output: 3768

Expand Down
1 change: 1 addition & 0 deletions packages/theme/src/month-table/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@

&:not(.in-range) {
.cell {
box-sizing: border-box;
width: auto;
border-radius: var(--tv-MonthTable-cell-border-radius);
}
Expand Down
Loading