-
Notifications
You must be signed in to change notification settings - Fork 1
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
Adds icon for validated item of history #1856
Conversation
[auto-generated message] E2E test report on: https://algorea-static.s3.eu-central-1.amazonaws.com/branch/feature/add-icon-in-front-validated/playwright-report/index.html |
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 I would prefer if the icon was not duotone and would have the same size as the score ring (32x32?). Could you test that?
[auto-generated message] E2E test report on: https://algorea-static.s3.eu-central-1.amazonaws.com/branch/feature/add-icon-in-front-validated/playwright-report/index.html |
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.
[auto-generated message] E2E test report on: https://algorea-static.s3.eu-central-1.amazonaws.com/branch/feature/add-icon-in-front-validated/playwright-report/index.html |
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.
314a0e7
to
5cddbf7
Compare
[auto-generated message] E2E test report on: https://algorea-static.s3.eu-central-1.amazonaws.com/branch/feature/add-icon-in-front-validated/playwright-report/index.html |
I've updated the test case, but didn't mention it, try updated link from usual user |
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.
Apply the same changes to the group view. (btw, I don't get why the code is so duplicated between these 2 components...)
src/app/pipes/logActivityTypeIcon.ts
Outdated
pure: true, | ||
standalone: true | ||
}) | ||
export class LogActivityTypeIcon implements PipeTransform { |
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.
Usually, we try to suffix pipe classes by "Pipe"
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.
Fixed
[auto-generated message] E2E test report on: https://algorea-static.s3.eu-central-1.amazonaws.com/branch/feature/add-icon-in-front-validated/playwright-report/index.html |
I've checked those components, the difference I see are:
|
I've add new test case for group logs |
Description
Fixes #1849
Notes (out of scope, known isues, hints for reviewing code, ...) (optional)
It looks smaller but size is the same as other icons - 24px
Test cases
Case 1:
Case 2: