-
Notifications
You must be signed in to change notification settings - Fork 481
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
Update "Resolved" styling for code review comments #43135
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.
LGTM!
@@ -89,17 +89,18 @@ class Comment extends Component { | |||
<div | |||
style={{ | |||
...styles.commentContainer, | |||
...(isFromOlderVersionOfProject && | |||
styles.olderVersionCommentTextColor) | |||
...((isFromOlderVersionOfProject || isResolved) && styles.lessVisible) |
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.
nit: might be worth moving isFromOlderVersionOfProject || isResolved
to a local variable
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 should note that isFromOlderVersionOfProject
is always false b/c we never implemented this:
isFromOlderVersionOfProject: false |
I'll add a tech debt item for this to delete references to it (or is this something we're planning on implementing @jmkulwik)?
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.
It is something we plan to implement eventually (after launch), but probably not something we should be maintaining currently.
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.
Got it, added to tech debt epic here:
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.
LGTM, just one nit
iconClass: 'fa fa-fw fa-check', | ||
text: javalabMsg.resolve(), | ||
iconClass: 'fa fa-fw fa-check-circle', | ||
text: javalabMsg.markComplete(), | ||
key: 'Resolve' |
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.
nit: should we change this key (and the re-open key below)?
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'll go ahead and update this in the next PR.
This updates the styling of code review comments to be the "expanded" version of the expand/collapse comment.
Up next: Support collapsing and expanding comments
The new styling
Note: The teacher name is still displayed in blue in order to visually differentiate from a student.
Links
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: