[Peer Review] Review Panel UI Polish #1539
Conversation
zhangvi7
changed the title
| # Add reviewer as viewer | ||
| QueryExecutionViewer.create( | ||
| fields={ | ||
| "query_execution_id": query_execution_id, | ||
| "uid": reviewer_id, | ||
| "created_by": query_execution.uid, # Original query review creator | ||
| }, | ||
| commit=False, | ||
| session=session, | ||
| ) |
There was a problem hiding this comment.
check if reviewer has access to the query before assigning them. they might have gotten access if they are already invited to the data doc
| 50, | ||
| 'Please provide a detailed justification (minimum 50 characters)' |
| size={12} | ||
| className="mr4" | ||
| /> | ||
| {review.status.charAt(0).toUpperCase() + review.status.slice(1)} |
There was a problem hiding this comment.
import { capitalize } from 'lodash';
| const visibleReviewers = review.reviewer_ids.slice(0, 3); | ||
| const extraReviewers = Math.max(0, review.reviewer_ids.length - 3); |
There was a problem hiding this comment.
do this inside MetaInfo, these arent passed other places
also useMemo to prevent a new array generated per render cycle
querybook/webapp/components/QueryReviewsNavigator/QueryReviewsNavigator.tsx
Show resolved
Hide resolved
| export const selectMyReviewsPage = createSelector( | ||
| selectQueryReviewState, | ||
| (queryReview) => queryReview.myReviewsPage | ||
| ); | ||
|
|
||
| export const selectAssignedReviewsPage = createSelector( | ||
| selectQueryReviewState, | ||
| (queryReview) => queryReview.assignedReviewsPage | ||
| ); | ||
|
|
||
| export const selectMyReviewsHasMore = createSelector( | ||
| selectQueryReviewState, | ||
| (queryReview) => queryReview.myReviewsHasMore | ||
| ); | ||
|
|
||
| export const selectAssignedReviewsHasMore = createSelector( | ||
| selectQueryReviewState, | ||
| (queryReview) => queryReview.assignedReviewsHasMore | ||
| ); |
There was a problem hiding this comment.
no point of these selectors given there is no further computation needed. Use state.queryReview.assignedReviewsHasMore directly in code
| myReviewsPage: number; | ||
| myReviewsHasMore: boolean; | ||
| assignedReviewsPage: number; | ||
| assignedReviewsHasMore: boolean; |
There was a problem hiding this comment.
not sure if this scalable, also we are resetting when tabs are changed anyway, its fine we just have page and hasMore
| getReviewsCreatedByMe: (limit: number, offset: number) => | ||
| ds.fetch<IQueryReview[]>('/query_review/created_by_me/', { | ||
| limit, | ||
| offset, | ||
| }), | ||
|
|
||
| getReviewsAssignedToMe: () => | ||
| ds.fetch<IQueryReview[]>('/query_review/assigned_to_me/'), | ||
| getReviewsAssignedToMe: (limit: number, offset: number) => | ||
| ds.fetch<IQueryReview[]>('/query_review/assigned_to_me/', { | ||
| limit, | ||
| offset, | ||
| }), |
There was a problem hiding this comment.
we have 2 routes that does the same thing? let's use a generic route /query_review/<review_type>/
| size={12} | ||
| className="mr4" | ||
| /> | ||
| {capitalize(review.status.charAt(0)) + review.status.slice(1)} |
| try { | ||
| dispatch({ type: FETCH_MY_REVIEWS_REQUEST }); | ||
| const reviews = await QueryReviewResource.getReviewsCreatedByMe(); | ||
| const reviews = await QueryReviewResource.getReviews( |
There was a problem hiding this comment.
we don't need fetchMyReviews and fetchAssignedReviews, it can be the same function with a parameter of ReviewType
|
|
||
| type TabType = 'myReviews' | 'assigned'; | ||
|
|
||
| const NAVIGATOR_TABS = [ | ||
| { | ||
| key: 'myReviews' as TabType, | ||
| name: 'Pending Reviews', | ||
| name: 'Created', |
There was a problem hiding this comment.
lets make the naming more consistent, right now we have myReviews and Created mixed together (ReviewType is using CREATED instead of myReviews), let's just stick to one
| type: isMyReviews | ||
| ? FETCH_MY_REVIEWS_FAILURE | ||
| : FETCH_ASSIGNED_REVIEWS_FAILURE, |
There was a problem hiding this comment.
why not forward ReviewType for all of these cases?
Before
After
