Skip to content

Improved the upcoming events and notification section of dashboard screen #298

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 13 commits into from
Closed

Improved the upcoming events and notification section of dashboard screen #298

wants to merge 13 commits into from

Conversation

harsh253
Copy link
Member

@harsh253 harsh253 commented Feb 9, 2020

Problem
To improve the frontend of the dashboard screen

Issue this PR referred to
Fixes issue number 295. Refer to this https://github.com/codeuino/social-platform-donut-frontend/issues/295 for more details.

Solution of problem
I have added images , icons and other content in the upcoming event and notification section. Have also modified the height of these sections to better align with the given designs.
I have made changes to notification.js, notification.scss, portfolio.scss, upcoming-event.js and upcoming-event.scss. Also added some images in the image folder.

Before changes were made
Screenshot from 2020-02-09 18-31-52

After changes were made
Screenshot from 2020-02-09 23-31-51

@jaskiratsingh2000
Copy link
Member

jaskiratsingh2000 commented Feb 9, 2020 via email

@vaibhavdaren
Copy link
Member

why does your pull requests contain of older commits that too by other people.

@harsh253
Copy link
Member Author

I think those are the changes in the readme.md file which were giving merge conflicts initially.

Rupeshiya
Rupeshiya reviewed Feb 10, 2020
View reviewed changes
Copy link
Member

@Rupeshiya Rupeshiya left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@harsh253 I think instead of writting the hard coded code again and again , you can use store the data in an array and simply use map method in react for iterating over it. which will reduce the repeation of same code again and again, and also this make sense coz we are going to use the data from the API which will be in the form of an array of objects,
something like this :

const events = [
{
  _id: 1,
  eventName: 'Testing event',
  createdBy: 'Test'
 .......
},
{
  _id: 2,
  eventName: 'Testing event2',
  createdBy: 'Test2'
 .......
}
];

Iterate over it

let events = this.events.map(event => (
     <ul key={event._id}>
      <li>{event.eventName}</li> 
   </ul>
));

In jsx part
{events}

@vaibhavdaren @devesh-verma please look the code once.

@harsh253
Copy link
Member Author

Hi @Rupeshiya . Thank you for your feedback.
Initially I had thought of doing it the same way as you are telling but then I went through the updates.js file which included repetition of code which is when I decided to go by the same method.
So if this needs an improvement , I can refactor the code and send a new PR

@Rupeshiya
Copy link
Member

Cool @harsh253 , @vaibhavdaren @devesh-verma plz review.

devesh-verma
devesh-verma requested changes Feb 11, 2020
View reviewed changes
Copy link
Member

@devesh-verma devesh-verma left a comment

Choose a reason for hiding this comment

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

@harsh253 what @Rupeshiya mentioned is true, it will be better if you can do it the way he mentioned. Also, I can see your style for notification area and upcoming events are getting applied to the rightmost column. Please take care of these.

@harsh253
Copy link
Member Author

Hi @devesh-verma. Thank you for the feedback. I will make the changes and send a new PR for it.
Also in the rightmost column I've made changes deliberately to make it look as similar as possible to the designs. If you want , I can revert back those changes.

@netlify
Copy link

netlify bot commented Feb 12, 2020 •
edited
Loading

Deploy preview for donut-frontend-r ready!

Built with commit 175e4fd

https://deploy-preview-298--donut-frontend-r.netlify.com

@Rupeshiya
Copy link
Member

@harsh253 The PR is having the conflict, please resolve it.

@harsh253 harsh253 closed this Feb 12, 2020
@harsh253 harsh253 reopened this Feb 12, 2020
@harsh253
Copy link
Member Author

Hi @Rupeshiya. Can I close this PR and open a new PR because I think there are some unwanted changes that I am unable to resolve .

@devesh-verma
Copy link
Member

@harsh253 Hey I am closing the PR. Please send in a new PR. Make sure to user development branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rupeshiya Rupeshiya Rupeshiya left review comments

@devesh-verma devesh-verma devesh-verma requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@harsh253 @jaskiratsingh2000 @vaibhavdaren @Rupeshiya @devesh-verma @vinitshahdeo