-
Notifications
You must be signed in to change notification settings - Fork 394
fix: add cx-read-more component (for product reviews) #20250
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
Conversation
spartacus
|
Project |
spartacus
|
Branch Review |
feature/CXSPA-9967
|
Run status |
|
Run duration | 05m 09s |
Commit |
|
Committer | Uros Lates |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
3
|
|
2
|
|
0
|
|
249
|
View all changes introduced in this branch ↗︎ |
projects/storefrontlib/shared/components/read-more/read-more.component.ts
Show resolved
Hide resolved
import { I18nModule } from '@spartacus/core'; | ||
|
||
/** | ||
* Renders cx-read-more. |
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.
This is a nice approach to the read more case. Simple enough to understand quickly and works well. Could we expand on jsdocs for the class and properties?
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.
Applied! 👍
@@ -43,12 +46,16 @@ export class ProductReviewsComponent { | |||
@ViewChild('writeReviewButton', { static: false }) | |||
writeReviewButton: ElementRef; | |||
|
|||
@Input() inputCharactersForReviewTitle = 255; |
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.
We should keep this behaviour feature toggled as I would consider it a breaking change. If it falls back to null
, then I think the maxLength
property wouldn't be applied.
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.
I think the property name could also be changed to something closer to the attribute it affects like maxLengthReviewTitle
. Just an example, but I think it could be more clear.
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.
Addressed! 👍
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.
Applied! 👍
@@ -127,12 +134,53 @@ export class ProductReviewsComponent { | |||
} | |||
} | |||
|
|||
get reviewTitleCharacterLeft(): number { |
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.
I'm not a huge fan of using getters this way as they retrigger on every component interaction (eg. clicking on the field triggers all, typing a character triggers all, etc). Its a big impact on performance at scale. It would be better if they could trigger only when the needed property changes.
Note: It could be an opportunity to look into the new angular signals.


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.
Applied 👍. Removed those and moved to template.
...efrontlib/cms-components/product/product-tabs/product-reviews/product-reviews.component.html
Show resolved
Hide resolved
Merge Checks Failed
|
Closes https://jira.tools.sap/browse/CXSPA-9967