fix: css alignment of calendar component on the home page #3276
Conversation
dishafaujdar
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe pull request introduces changes to several components, primarily enhancing the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
✅ Deploy Preview for asyncapi-website ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3276--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
components/Calendar.tsx (2)
39-39: LGTM: Addition of 'calendar-title' class with a minor suggestionThe addition of the 'calendar-title' class to the Heading component is good for specific styling of the calendar title. This change aligns with the PR objective of addressing alignment issues.
Consider using template literals for better readability when combining multiple classes:
-<Heading level={HeadingLevel.h2} typeStyle={HeadingTypeStyle.mdSemibold} className='calendar-title'> +<Heading level={HeadingLevel.h2} typeStyle={HeadingTypeStyle.mdSemibold} className={`calendar-title ${styles.heading}`}>This approach allows for easier addition of conditional classes in the future if needed.
Line range hint
1-70: Overall assessment: Changes align with PR objectives and improve component consistencyThe modifications made to the Calendar component, including the addition of specific classes for styling and the standardization of the test ID, align well with the PR objectives of addressing alignment issues. These changes improve the overall consistency and maintainability of the component.
To further enhance the component's modularity and reusability, consider extracting the styling classes into a separate CSS module or a styled-component. This would allow for easier management of styles and potential theming in the future.
styles/globals.css (1)
126-140: LGTM! Consider a minor adjustment for better maintainability.The new media query effectively addresses the alignment issue for smaller screens (300px to 400px), which aligns with the PR objectives. The styles improve responsiveness and maintain consistency in the calendar section.
For better maintainability, consider using CSS variables for the font sizes and spacing values. This would make it easier to adjust these values globally in the future. For example:
:root { --calendar-font-size-small: 25px; --calendar-padding-small: 30px 35px; /* ... other variables ... */ } @media (min-width: 300px) and (max-width: 400px) { .calendar-container { padding: var(--calendar-padding-small); /* ... other styles ... */ } .calendar-title, .calendar-button { font-size: var(--calendar-font-size-small); } }This approach would make it easier to maintain consistent sizes across different components and media queries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- components/Calendar.tsx (2 hunks)
- next-env.d.ts (1 hunks)
- styles/globals.css (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- next-env.d.ts
🧰 Additional context used
🔇 Additional comments (3)
components/Calendar.tsx (2)
33-38: LGTM: Addition of 'calendar-container' classThe addition of the 'calendar-container' class to the outer div is a good approach for targeted styling. The use of
twMergeensures that there are no class conflicts. This change aligns well with the PR objective of addressing alignment issues.
61-61: LGTM: Standardization of data-testid attribute with a cautionThe change from 'Calendar-button' to 'calendar-button' for the data-testid attribute improves consistency in the naming convention. This is a good practice for maintaining a standardized codebase.
However, this change might potentially break existing tests that rely on the old test ID. Please ensure that all related tests are updated accordingly. Run the following script to check for any occurrences of the old test ID in test files:
If any occurrences are found, update them to use the new 'calendar-button' test ID.
✅ Verification successful
LGTM: Standardization of
data-testidAttribute ConfirmedThe change from
'Calendar-button'to'calendar-button'for thedata-testidattribute enhances consistency within the codebase. No existing tests depend on the old test ID, ensuring that this update does not disrupt any test functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for occurrences of the old test ID in test files # Test: Search for 'Calendar-button' in test files rg --type-add 'test:*.{test,spec}.{js,jsx,ts,tsx}' --type test 'Calendar-button'Length of output: 80
styles/globals.css (1)
126-140: Overall, these changes effectively address the alignment issue.The new media query for screen sizes between 300px and 400px successfully targets the alignment problem described in the PR objectives. The changes are minimal and focused, which reduces the risk of unintended side effects. The styles are consistent with the existing code structure and follow the established patterns in the file.
To ensure these changes don't negatively impact other parts of the website, please run the following verification script:
This script will help identify any potential conflicts or unintended overrides in other CSS files.
✅ Verification successful
Verification Successful: No Conflicts Found
The newly added media query in
styles/globals.cssdoes not conflict with any existing media queries. Additionally, the affected classes (calendar-container,calendar-title,calendar-button) are consistently used in bothstyles/globals.cssandcomponents/Calendar.tsx, ensuring that there are no unintended overrides or style conflicts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any conflicting media queries or overlapping styles # Test: Search for other media queries that might conflict with the new one echo "Checking for potentially conflicting media queries:" rg --type css "media.*and.*max-width.*400px" --glob '!styles/globals.css' # Test: Search for other occurrences of the affected classes to ensure no unintended overrides echo "Checking for other occurrences of the affected classes:" rg --type css "calendar-(container|title|button)" --glob '!styles/globals.css'Length of output: 688
Script:
#!/bin/bash # Description: Retry checking for conflicting media queries or overlapping styles without overly restrictive filters # Test: Search for media queries that might conflict with the new one, excluding styles/globals.css echo "Checking for potentially conflicting media queries:" rg "media.*and.*max-width.*400px" --glob '!.git/**' --glob '!node_modules/**' # Test: Search for other occurrences of the affected classes to ensure no unintended overrides echo "Checking for other occurrences of the affected classes:" rg "calendar-(container|title|button)" --glob '!.git/**' --glob '!node_modules/**'Length of output: 923
|
How about doing only - <Calendar size={2} className='float-left' />
+ <Calendar size={2} className='content-center' />in
|
.gitignore
Outdated
| @@ -22,3 +22,4 @@ cypress/videos | |||
| /storybook-static/ | |||
| coverage | |||
| deno.lock | |||
| /styles/globals.css | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency Detected with styles/globals.css Tracking
The addition of /styles/globals.css to .gitignore is inconsistent because:
styles/globals.cssexists and is tracked by Git.- It is referenced in
pages/_app.tsx.
This implies that ignoring it in .gitignore has no effect and may lead to confusion or unintended behavior. Please review the necessity of this .gitignore entry.
🔗 Analysis chain
Verify the intention of ignoring styles/globals.css
The addition of /styles/globals.css to .gitignore is a valid change. However, please confirm if this is intentional and consider the following implications:
- Changes to
globals.csswill no longer be tracked by Git, which could lead to inconsistencies across different environments. - Other developers might lose access to important global styles.
Also, there seems to be an inconsistency:
- The PR objectives mention modifications to
globals.css. - The AI summary states that
styles/globals.csshas been removed. - Yet, we're adding it to
.gitignore, which implies the file still exists but should be ignored.
Could you clarify the status and intention for styles/globals.css?
To help verify the status of styles/globals.css, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the status of styles/globals.css
# Test 1: Check if the file exists
if [ -f "styles/globals.css" ]; then
echo "styles/globals.css exists"
else
echo "styles/globals.css does not exist"
fi
# Test 2: Check Git status of the file
git status --porcelain styles/globals.css
# Test 3: Check if the file is tracked by Git
git ls-files --error-unmatch styles/globals.css 2>/dev/null
if [ $? -eq 0 ]; then
echo "styles/globals.css is tracked by Git"
else
echo "styles/globals.css is not tracked by Git"
fi
# Test 4: Check for references to globals.css in other files
echo "Files referencing globals.css:"
rg "globals\.css" --type css --type html --type javascript --type typescript
Length of output: 491
Script:
#!/bin/bash
# Description: Re-verify the status of styles/globals.css with corrected file type identifiers
# Test 1: Check if the file exists
if [ -f "styles/globals.css" ]; then
echo "styles/globals.css exists"
else
echo "styles/globals.css does not exist"
fi
# Test 2: Check Git status of the file
git status --porcelain styles/globals.css
# Test 3: Check if the file is tracked by Git
git ls-files --error-unmatch styles/globals.css 2>/dev/null
if [ $? -eq 0 ]; then
echo "styles/globals.css is tracked by Git"
else
echo "styles/globals.css is not tracked by Git"
fi
# Test 4: Check for references to globals.css in other files
echo "Files referencing globals.css:"
rg "globals\.css" --type css --type html --type js --type ts
Length of output: 487
|
@dishafaujdar Use more descriptive title for the PR. Something like 'Fix CSS alignment of calendar component on the home page' will work great. |
dishafaujdar
changed the title
|
@devilkiller-ag I'll keep that in mind. |
| <div className={twMerge('overflow-hidden rounded-md border border-gray-200 bg-white p-4', className)}> | ||
| <Heading level={HeadingLevel.h2} typeStyle={HeadingTypeStyle.mdSemibold}> | ||
| <div | ||
| className={twMerge( |
There was a problem hiding this comment.
In the mobile/tab view, the calendar div is not in the center, rather it is slightly shifted towards the right.
It should be in the center like other cards:
|
Can workflows be approved and an approving comment posted once again after they run, please? |
pages/[lang]/index.tsx
Outdated
| @@ -108,7 +108,7 @@ export default function HomePage() { | |||
|
|
|||
| <div className='mt-12 lg:flex lg:flex-row-reverse'> | |||
| <section className='mt-10 lg:mt-0 lg:flex-1'> | |||
| <Calendar size={2} className='float-left' /> | |||
| <Calendar size={2} className='content-center' /> | |||
There was a problem hiding this comment.
In this way, all the content inside component will be centred, but we only want the content to be centred, when there are no meetings present.
There was a problem hiding this comment.
@akshatnema I am working on the change that you've suggested.
|
@dishafaujdar |
dishafaujdar
force-pushed
the
CssAlign
branch
from
00358bb to
c49a672
Compare
asyncapi-bot
requested review from
devilkiller-ag,
BhaswatiRoy,
J0SAL and
TRohit20
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
package.json (1)
72-72: Consider consolidating date manipulation libraries.The project already includes
moment.js. Addingdate-fnscould lead to inconsistent date handling across the codebase. Consider:
- Using the existing moment.js library instead
- Or migrating fully to date-fns (recommended as moment.js is in maintenance mode)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
components/Calendar.tsx(4 hunks)package.json(1 hunks)tests/build-tools.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/Calendar.tsx
🔇 Additional comments (2)
tests/build-tools.test.js (1)
29-29: LGTM, but verify if explicit String conversion is necessary.
While the change is safe, os.tmpdir() already returns a string in Node.js. If this was added for TypeScript type safety, consider reviewing your TypeScript configuration instead.
Let's verify the return type documentation:
package.json (1)
72-72: Verify if date-fns is necessary for this PR.
This PR's primary objective is to fix CSS alignment issues with the calendar component. Adding a new date manipulation library seems outside the scope of these changes.
✅ Verification successful
The addition of date-fns is justified and properly used
The verification shows that date-fns is actively used in the Calendar component for date comparison logic (isAfter and parseISO functions), while moment.js is used for date formatting. The library serves a distinct purpose from moment.js in this component, specifically for comparing event dates with the current date. While this PR was primarily about CSS fixes, the date-fns addition appears to be a necessary part of the calendar component's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if date-fns is actually used in the calendar component changes
# Test: Look for date-fns usage in calendar-related files
rg -l 'date-fns' | grep -i 'calendar'
# Test: Check the specific functions mentioned in the summary
rg -l 'isAfter|parseISO' | grep -i 'calendar'
Length of output: 129
Script:
#!/bin/bash
# Let's check the actual usage of date-fns in Calendar.tsx
rg -A 3 -B 3 'date-fns' components/Calendar.tsx
# Also check for any moment.js usage in the same file
rg -A 3 -B 3 'moment' components/Calendar.tsx
# Look at the specific changes in Calendar.tsx
ast-grep --pattern 'import { $_ } from "date-fns"'
Length of output: 1278
Script:
#!/bin/bash
# Let's see where date-fns functions are actually used in the component
rg -A 5 'isAfter|parseISO' components/Calendar.tsx
# Also check the git diff to understand the changes
git diff origin/main components/Calendar.tsx
Length of output: 632
|
@akshatnema
|
|
@aeworxet It will be better if you target this in another PR. Create an issue regarding this and then try to make this in separate PR, without going ahead in this PR's context |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
components/Calendar.tsx (1)
Line range hint
52-56: Standardize date formatting across the componentThe component uses different date formatting patterns:
moment(event.date).format('D')for the circle displaymoment(event.date).local().format('LLLL')for the full datemoment(event.date).local().format('Z')for timezoneConsider standardizing these formats and extracting them into constants or utility functions for better maintainability.
+const DATE_FORMATS = { + DAY: 'D', + FULL_DATE: 'LLLL', + TIMEZONE: 'Z' +}; export default function Calendar({ className = '', size }: ICalendarProps) { // ... existing code ... // Example usage: - moment(event.date).format('D') + moment(event.date).format(DATE_FORMATS.DAY)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/Calendar.tsx(3 hunks)
🔇 Additional comments (3)
components/Calendar.tsx (3)
31-32: Skip comment as covered by past review
62-67: Skip comment as covered by past review
52-52: Verify heading hierarchy
While using semantic headings is good for accessibility, let's verify that the h2 tag here doesn't create an invalid heading hierarchy, as there's already an h2 heading above in the calendar title.
| <div | ||
| className={twMerge( | ||
| 'overflow-hidden rounded-md border border-gray-200 bg-white p-4 h-full flex flex-col gap-2', | ||
| className | ||
| )} | ||
| > |
There was a problem hiding this comment.
Address mobile view alignment issue
While the flex container improvements help with layout, the mobile view alignment issue mentioned in the past review still needs attention. The calendar div appears shifted to the right on mobile/tab views instead of being centered like other cards.
Apply this diff to center the component on mobile views:
<div
className={twMerge(
- 'overflow-hidden rounded-md border border-gray-200 bg-white p-4 h-full flex flex-col gap-2',
+ 'overflow-hidden rounded-md border border-gray-200 bg-white p-4 h-full flex flex-col gap-2 mx-auto w-full',
className
)}
>📝 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.
| <div | |
| className={twMerge( | |
| 'overflow-hidden rounded-md border border-gray-200 bg-white p-4 h-full flex flex-col gap-2', | |
| className | |
| )} | |
| > | |
| <div | |
| className={twMerge( | |
| 'overflow-hidden rounded-md border border-gray-200 bg-white p-4 h-full flex flex-col gap-2 mx-auto w-full', | |
| className | |
| )} | |
| > |
|
@dishafaujdar Can we have a gap between the text and the button? |
This happened due to my commit of a CodeRabbit code suggestion. I removed it. |
|
Can this PR be merged, or is there anything else that needs to be done on it? The change request #3276 (review) is outdated. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/build-tools.test.js (1)
29-29: Fix indentation to match project style.The line has incorrect indentation according to the project's style guide.
Apply this diff to fix the indentation:
- const testDir = path.join(String(os.tmpdir()), 'test_config'); + const testDir = path.join(String(os.tmpdir()), 'test_config');🧰 Tools
🪛 eslint (1.23.1)
[error] 29-29: Delete
··(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
tests/build-tools.test.js(1 hunks)
🧰 Additional context used
🪛 eslint (1.23.1)
tests/build-tools.test.js
[error] 29-29: Delete ··
(prettier/prettier)
🔇 Additional comments (1)
tests/build-tools.test.js (1)
29-29: Verify the necessity of explicit string conversion.
While the explicit string conversion is a defensive programming practice, it would be helpful to understand the specific scenario that necessitated this change, as os.tmpdir() typically returns a string.
Let's verify the return type of os.tmpdir() across different Node.js versions:
🧰 Tools
🪛 eslint (1.23.1)
[error] 29-29: Delete ··
(prettier/prettier)
|
/rtm |
Fix #3275
Describe the bug.
Alignment :
Calendar Card :
Summary by CodeRabbit
Release Notes
New Features
Calendarcomponent to better display event availability, with improved layout and message clarity when no events are present.Localization Updates
Bug Fixes
Calendarcomponent to ensure consistent styling and layout.These updates aim to improve user experience by providing clearer information and a more responsive design.