Peer review ui 2 [test ui]#9293
Conversation
|
|
bf81e5e to
f663d2d
Compare
There was a problem hiding this comment.
We had discussed locked being a property on the puzzle dot, not something specific to peer review.
There was a problem hiding this comment.
Right now, the only circumstance where a level would be locked is if its a locked peer review slot.
I can add a property the levels on whether or not a certain line is locked. Will try that out
|
A few spec-level suggestions for peer review:
|
|
Tagging @ryansloan for comment on above |
|
Re: the Horizontal List, I like it more from a visual standpoint (especially because it eliminates the repeated strings), but I worry that it could be a bit ambiguous. It's not necessarily clear why a given peer review is locked. Now I'm inventing features, but maybe we could do the row of dots with the status in a tooltip when you hover/click on a locked level? |
|
I propose leaving it as in, then taking feedback from our bug bash next week and focus group the week after. I'd like to better understand if people plan on coming back to peer reviews they've done, what order they go in when doing them, and what's clear / not clear about expectations for this section. |
f663d2d to
ddb75f5
Compare
ddb75f5 to
6cb1aa6
Compare
|
Implemented most of the suggestions here - progress_dot.jsx isn't as messy as I thought it would be |
| url: '', | ||
| name: I18n.t('peer_review.reviews_unavailable'), | ||
| icon: 'fa-lock' | ||
| icon: 'fa-lock', |
There was a problem hiding this comment.
Let's have locked imply the 'fa-lock' icon so this doesn't need to be passed separately.
There was a problem hiding this comment.
I'm not sure...I guess it depends on what we'd want to happen if we locked a level that had an icon.
There was a problem hiding this comment.
I guess that's possible, but in that case I think we'd still want to show the default locked styling (so that the level being locked is more important that what type of level it is).
|
Still seeing "Review in progress" for reviews I haven't started yet:
Edit: this definitely shouldn't block check-in, can be part of a future PR. |
| }); | ||
| export default connect(state => ({ | ||
|
|
||
| export default connect((state) => ({ |
There was a problem hiding this comment.
Super small nit: extra parens diff isn't needed.
|
This is looking awesome! My main feedback this pass is to make the peer review dot styling as close as possible to task & make |
6e0cde6 to
4cfa104
Compare
4cfa104 to
9fa1068
Compare
| id: id, | ||
| url: level.url | ||
| }); | ||
| })})); |
There was a problem hiding this comment.
FYI, I have an open PR that might result in some merge conflicts with this. #9379
|
The test coverage you've written looks good, but it would be great to have a UI test to cover this scenario end-to-end. Doesn't need to block this PR. |
|
My priority right now is getting something that the team can bash on Thursday, and that I can give to facilitators next Wednesday. After that, I'd like to make an eyes test to verify the appearance of unit view for someone who's partially done with stuff |
|
Hmm I'm getting the following after completing a peer review: |
|
LGTM! (After resolving |
746e14b to
71f176f
Compare
…inally Revert "Merge pull request #9293 from code-dot-org/peer_review_ui_2"
UI for peer review, but without more react classes
Different enough to justify a new PR