Copilot commented on code in PR #993:
URL:
https://github.com/apache/dolphinscheduler-website/pull/993#discussion_r2772522223
##########
src/components/Footer/index.scss:
##########
@@ -23,11 +29,25 @@
}
&-links {
- height: 108px;
+ min-height: 80px;
+ height: auto;
display: flex;
- justify-content: space-between;
+ justify-content: center;
Review Comment:
The footer links are centered with `justify-content: center` on all screen
sizes, but the original design likely had them spread across the footer width
with `justify-content: space-between`. This changes the visual layout
significantly. If centering is intentional for better mobile responsiveness,
consider keeping `space-between` on larger screens and only switching to
`center` on smaller screens where wrapping occurs.
##########
src/views/Home/tasks.scss:
##########
@@ -45,84 +46,110 @@
@include content-width;
}
+
&-title {
font-weight: 700;
color: #031b3e;
font-size: 48px;
display: flex;
+ align-items: baseline;
+ justify-content: center;
+ gap: 12px;
Review Comment:
The title alignment is set to `align-items: baseline` which may cause
alignment issues when the text wraps. Consider using `align-items: center`
instead, or add a media query to switch to center alignment on smaller screens
where wrapping is more likely to occur.
##########
src/views/Home/index.scss:
##########
@@ -5,82 +5,202 @@
display: flex;
flex-direction: column;
align-items: center;
+ overflow-x: hidden;
+
&-desc {
@include content-width;
- }
- &-github-buttons {
- margin-top: 42px;
- }
- .github-button {
+ padding: 0 20px;
Review Comment:
The `content-width` mixin already applies `padding: 0 2vw` for screens under
1280px. Adding `padding: 0 20px` here will override that padding, resulting in
fixed 20px padding instead of the responsive 2vw padding. On a 1280px screen,
2vw equals 25.6px, which is close to 20px. However, on smaller screens like
640px, 2vw equals 12.8px, but this code forces 20px, which may be too much
padding on mobile devices. Consider using the mixin's existing responsive
padding or adjusting it conditionally with media queries.
```suggestion
@media screen and (min-width: 1281px) {
padding: 0 20px;
}
```
##########
src/views/Home/community.scss:
##########
@@ -1,70 +1,168 @@
.home-community {
- padding-top: 100px;
- &-title {
- font-weight: 700;
- color: #031b3e;
- font-size: 50px;
- text-align: center;
- padding-bottom: 60px;
-
- @media screen and (max-width: 640px) {
- font-size: 7vw;
- }
+ padding-top: 100px;
+ width: 100%;
+
+ @media screen and (max-width: 768px) {
+ padding-top: 60px;
+ }
+
+ @media screen and (max-width: 640px) {
+ padding-top: 40px;
+ }
+
+ &-title {
+ font-weight: 700;
+ color: #031b3e;
+ font-size: 50px;
+ text-align: center;
+ padding-bottom: 60px;
+
+ @media screen and (max-width: 768px) {
+ font-size: 36px;
+ padding-bottom: 40px;
}
- &-content {
- display: flex;
- justify-content: space-between;
+
+ @media screen and (max-width: 640px) {
+ font-size: 28px;
+ padding-bottom: 30px;
+ }
+
+ @media screen and (max-width: 480px) {
+ font-size: 24px;
}
-
- &-total {
+ }
+
+ &-content {
+ display: flex;
+ justify-content: space-between;
+ align-items: center;
+ gap: 40px;
+
+ @media screen and (max-width: 1024px) {
+ flex-direction: column;
+ }
+ }
+
+ &-info {
+ flex-shrink: 0;
+
+ @media screen and (max-width: 1024px) {
display: flex;
+ flex-direction: column;
align-items: center;
text-align: center;
}
- &-item {
- padding-right: 50px;
+ }
+
+ &-total {
+ display: flex;
+ align-items: center;
+ text-align: center;
+
+ @media screen and (max-width: 480px) {
+ flex-direction: column;
+ gap: 20px;
+ }
+ }
+
+ &-item {
+ padding-right: 50px;
+
+ @media screen and (max-width: 768px) {
+ padding-right: 30px;
+ }
+
+ @media screen and (max-width: 480px) {
+ padding-right: 0;
+ }
+ }
+
+ &-amount {
+ font-weight: 700;
+ color: var(--primary-color);
+ font-size: 60px;
+
+ @media screen and (max-width: 768px) {
+ font-size: 48px;
+ }
+
+ @media screen and (max-width: 640px) {
+ font-size: 40px;
+ }
+
+ @media screen and (max-width: 480px) {
+ font-size: 36px;
}
- &-amount {
- font-weight: 700;
+ }
+
+ &-text {
+ color: #000000;
+ font-size: 20px;
+ margin-top: 8px;
+
+ @media screen and (max-width: 768px) {
+ font-size: 18px;
+ }
+
+ @media screen and (max-width: 640px) {
+ font-size: 16px;
+ }
+ }
+
+ .ant-divider-vertical {
+ height: 80px;
+ border-color: #707070;
+ margin-right: 50px;
+
+ @media screen and (max-width: 768px) {
+ height: 60px;
+ margin-right: 30px;
+ }
+
+ @media screen and (max-width: 480px) {
+ display: none;
+ }
+ }
+
+ &-icons {
+ margin-top: 30px;
+ margin-bottom: 50px;
+
+ @media screen and (max-width: 640px) {
+ margin-top: 20px;
+ margin-bottom: 30px;
+ }
+ }
+
+ .anticon {
+ font-size: 30px;
+
+ @media screen and (max-width: 640px) {
+ font-size: 24px;
+ }
+
+ &:hover {
color: var(--primary-color);
- font-size: 60px;
-
- @media screen and (max-width: 640px) {
- font-size: 10vw;
- }
- }
- &-text {
- color: #000000;
- font-size: 20px;
- margin-top: 8px;
-
- @media screen and (max-width: 640px) {
- font-size: 16px;
- }
- }
- .ant-divider-vertical {
- height: 80px;
- border-color: #707070;
- margin-right: 50px;
- }
- &-icons {
- margin-top: 30px;
- margin-bottom: 50px;
- }
- .anticon {
- font-size: 30px;
- &:hover {
- color: var(--primary-color);
- }
- }
- .ant-image {
- @media screen and (max-width: 640px) {
- display: none;
- }
}
+ }
+
+ .ant-image {
+ max-width: 50%;
+
+ @media screen and (max-width: 1024px) {
+ max-width: 100%;
+ width: 100%;
+ }
+
+ img {
+ width: 100%;
+ height: auto;
+ }
+ }
Review Comment:
The change from hiding the image (`display: none`) to showing it
responsively (`max-width: 100%`) is an improvement for smaller screens.
However, verify that this doesn't cause performance issues on mobile devices,
especially if the images are large. Consider implementing lazy loading or using
responsive image sources (srcset) if the images are particularly large.
##########
src/views/Home/why.scss:
##########
@@ -4,68 +4,150 @@
.home-why {
@include content-width;
margin-top: 96px;
+ padding: 0 20px;
+ box-sizing: border-box;
+
Review Comment:
The `content-width` mixin already applies `padding: 0 2vw` for screens under
1280px. Adding `padding: 0 20px` here will override that padding, resulting in
fixed 20px padding instead of the responsive 2vw padding. On smaller screens
like 640px, 2vw equals 12.8px, but this code forces 20px, which may be
excessive. Consider using the mixin's existing responsive padding or adjusting
it conditionally with media queries.
```suggestion
box-sizing: border-box;
@media screen and (min-width: 1280px) {
padding: 0 20px;
}
```
##########
src/views/Home/tasks.scss:
##########
@@ -45,84 +46,110 @@
@include content-width;
}
+
&-title {
font-weight: 700;
color: #031b3e;
font-size: 48px;
display: flex;
+ align-items: baseline;
+ justify-content: center;
+ gap: 12px;
margin-top: 80px;
+ text-align: center;
+ flex-wrap: wrap;
+ line-height: 1.2;
@media screen and (max-width: 640px) {
font-size: 5vw;
+ margin-top: 24px;
+ line-height: 1.3;
}
}
+
&-total {
- padding-left: 20px;
- padding-right: 20px;
+ padding: 0 8px;
font-size: 80px;
+ line-height: 1;
position: relative;
- top: -30px;
+ top: 0;
+
@media screen and (max-width: 640px) {
font-size: 10vw;
- top: -2vw;
}
}
+
&-wrap {
display: flex;
width: 100%;
+ overflow: hidden;
}
+
&-animation {
display: flex;
+
&:hover {
animation-play-state: paused;
}
}
+
&-list {
display: flex;
padding-top: 40px;
+
+ @media screen and (max-width: 640px) {
+ padding-top: 24px;
+ }
}
+
&-item {
margin-left: 50px;
- display: inline-block;
+ display: inline-flex;
+ flex-direction: column;
+ align-items: center;
+
+ @media screen and (max-width: 640px) {
+ margin-left: 24px;
+ }
+
&-img {
width: 125px;
height: 125px;
background-repeat: no-repeat;
background-position: center;
@media screen and (max-width: 640px) {
- width: 16vw;
- height: 16vw;
- background-size: cover;
+ width: 56px;
+ height: 56px;
+ background-size: contain;
}
}
+
&-text {
color: #031b3e;
font-size: 12px;
line-height: 2;
text-align: center;
@media screen and (max-width: 640px) {
- width: 200%;
- transform: scale(0.8);
- margin-left: -50%;
+ font-size: 10px;
}
}
}
&-right {
- animation: 30s linear 0s infinite running slidein;
+ animation: 30s linear infinite slidein;
}
+
&-left {
- animation: 30s linear 0s infinite running slideout;
+ animation: 30s linear infinite slideout;
Review Comment:
The animation timing has been changed from `30s linear 0s infinite running
slidein` to `30s linear infinite slidein`. While both are functionally
equivalent (the `0s` delay and `running` state are defaults), ensure this
simplified syntax is consistent with project conventions and doesn't introduce
any unexpected behavior.
##########
src/views/Home/events.scss:
##########
@@ -68,13 +146,34 @@
font-size: 18px;
line-height: 32px;
margin-top: 20px;
- height: 128px;
- overflow: hidden;
+ flex: 1;
+
+ @media screen and (max-width: 768px) {
+ font-size: 16px;
+ line-height: 1.6;
+ margin-top: 15px;
+ }
+
+ @media screen and (max-width: 640px) {
+ font-size: 14px;
+ line-height: 1.5;
+ margin-top: 12px;
+ }
}
+
&-btn {
- position: absolute;
- bottom: 46px;
- right: 46px;
+ margin-top: 20px;
+
+ @media screen and (max-width: 1024px) {
+ position: static;
+ margin-top: 24px;
+ }
+
+ @media screen and (min-width: 1025px) {
+ position: absolute;
+ bottom: 46px;
+ right: 46px;
Review Comment:
The button positioning changes from absolute to static based on screen size,
which is good for responsive design. However, ensure that on screens >= 1025px
the absolute positioning doesn't cause the button to overlap with content when
the card content is shorter than expected. Consider testing with varying
content lengths.
```suggestion
position: static;
margin-top: 24px;
```
##########
src/views/Home/tasks.scss:
##########
@@ -45,84 +46,110 @@
@include content-width;
}
+
&-title {
font-weight: 700;
color: #031b3e;
font-size: 48px;
display: flex;
+ align-items: baseline;
+ justify-content: center;
+ gap: 12px;
margin-top: 80px;
+ text-align: center;
+ flex-wrap: wrap;
+ line-height: 1.2;
@media screen and (max-width: 640px) {
font-size: 5vw;
+ margin-top: 24px;
+ line-height: 1.3;
}
}
+
&-total {
- padding-left: 20px;
- padding-right: 20px;
+ padding: 0 8px;
font-size: 80px;
+ line-height: 1;
position: relative;
- top: -30px;
+ top: 0;
Review Comment:
The `top: 0` property effectively resets the original `-30px` positioning.
While this improves alignment on larger screens, verify that the visual
positioning of the task count number relative to surrounding text is acceptable
across all breakpoints. The original negative top value was likely intentional
for visual alignment.
##########
src/views/Home/index.scss:
##########
@@ -104,17 +234,18 @@
);
position: absolute;
top: 0px;
- left: -20px;
+ left: -10%;
z-index: 1;
- min-width: 1200px;
}
&-content {
position: relative;
z-index: 2;
margin: 0 auto;
+ padding: 0 20px;
+ box-sizing: border-box;
Review Comment:
The `content-width` mixin already applies `padding: 0 2vw` for screens under
1280px. Adding `padding: 0 20px` here will override that padding, resulting in
fixed 20px padding instead of the responsive 2vw padding. On smaller screens
like 640px, 2vw equals 12.8px, but this code forces 20px, which may be
excessive. Consider using the mixin's existing responsive padding or adjusting
it conditionally with media queries.
##########
src/views/Home/events.scss:
##########
@@ -6,49 +6,120 @@
margin-top: -100px;
position: relative;
z-index: 3;
+ padding: 0 20px;
Review Comment:
The `content-width` mixin already applies `padding: 0 2vw` for screens under
1280px. Adding `padding: 0 20px` here will override that padding, resulting in
fixed 20px padding instead of the responsive 2vw padding. On smaller screens
like 640px, 2vw equals 12.8px, but this code forces 20px, which may be
excessive. Consider using the mixin's existing responsive padding or adjusting
it conditionally with media queries.
```suggestion
```
##########
src/components/Connect/index.scss:
##########
@@ -42,9 +76,29 @@
font-size: 18px;
margin-top: 60px;
vertical-align: middle;
+ display: inline-flex;
+ align-items: center;
+ justify-content: center;
+ text-decoration: none;
+
+
Review Comment:
There's a trailing blank line in this CSS block. While not functionally
problematic, it's inconsistent with the rest of the codebase's formatting.
Consider removing the blank line for consistency.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
