Skip to content

Conversation

@gets0ul
Copy link
Contributor

@gets0ul gets0ul commented Jan 22, 2019

#2545
Show blue bar aside project in the project list (grid view) when it has any unread notifcations.

@maxceem maxceem self-requested a review January 22, 2019 06:03
Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gets0ul it's great that you reused existent functions, there are few moments to fix:

  1. ProjectsGridView is supposed to be a dummy component not a container. So it shouldn't be directly connected to the Redux store. I guess most of the logic can be moved to the nearer container src/projects/list/components/Projects/Projects.jsx and ProjectsGridView should only get everything it needs form the props.

  2. We have to show the blue line if there is any notification related to the project. filterProjectNotifications filters notifications for some particular types - we don't need this filter. filterNotificationsByProjectId is enough.

  3. Currently the blue line is only shown on the big screens where ID column is shown. But it dissapears on the smaller screens when ID column is hidden, see demo https://monosnap.com/file/t6XggYH4TNWrrZFIr9KFWDz0sA7YlC.
    Would you like to fix this and make the blue line visible on all screen sizes. We can rise the prize for additional $10 for this.

- Uses only filterNotificationsByProjectId as it is enough
- Keep the blue line visible
Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gets0ul, works great now.

Do you wanna make the third item? Or I will post it as a separate issue for everyone.

@gets0ul
Copy link
Contributor Author

gets0ul commented Jan 22, 2019

I thought I have fixed the third item?
Please check it on smaller screen. Thanks.

@maxceem
Copy link
Collaborator

maxceem commented Jan 22, 2019

Right, I've missed it, the third item is fixed, thanks!

@maxceem maxceem merged commit 10db6f2 into topcoder-archive:cf14 Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants