-
Notifications
You must be signed in to change notification settings - Fork 10
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.
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
480-refactor: Remove css prefixes #655
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also consider adding this rule?
https://stylelint.io/user-guide/rules/property-no-vendor-prefix/
Lighthouse Report:
|
This rule is already applied |
Are you sure this is coming from stylelint and not your ide? |
it's using |
it's somehow doesn't work if i use |
π WalkthroughWalkthroughThe changes in this pull request involve updates to the styling of 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
π§Ή Outside diff range and nitpick comments (1)
src/entities/event/ui/event-card/event-card.tsx (1)
37-37
: LGTM! Consider enhancing the title attribute.The addition of the title attribute improves accessibility for truncated text. However, you could make it even more user-friendly by providing more context.
Consider this enhancement:
- <p title={additionalInfo} className={cx('event-additional-info')}>{additionalInfo}</p> + <p title={`Additional event information: ${additionalInfo}`} className={cx('event-additional-info')}>{additionalInfo}</p>
π Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
π Files selected for processing (2)
src/entities/event/ui/event-card/event-card.module.scss
(1 hunks)src/entities/event/ui/event-card/event-card.tsx
(1 hunks)
π Additional comments (1)
src/entities/event/ui/event-card/event-card.module.scss (1)
88-88
:
Restore multi-line functionality using modern CSS
The change from -webkit-line-clamp
to white-space: nowrap
has reduced the content visibility from 2 lines to 1 line. Let's maintain the original functionality using modern CSS:
.event-additional-info {
overflow: hidden;
max-height: 40px;
margin-top: 0;
margin-bottom: 8px;
font-size: 16px;
text-overflow: ellipsis;
- white-space: nowrap;
+ display: -webkit-box;
+ -webkit-box-orient: vertical;
+ -webkit-line-clamp: 2;
}
Alternatively, consider using the modern line-clamp
property with appropriate fallbacks:
.event-additional-info {
overflow: hidden;
max-height: 40px;
margin-top: 0;
margin-bottom: 8px;
font-size: 16px;
text-overflow: ellipsis;
- white-space: nowrap;
+ display: -webkit-box;
+ -webkit-box-orient: vertical;
+ -webkit-line-clamp: 2;
+ line-clamp: 2;
}
Likely invalid or redundant comment.
What type of PR is this? (select all that apply)
Description
Related Tickets & Documents
Screenshots, Recordings
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?
Summary by CodeRabbit
New Features
title
attribute to the additional information text.Style