Skip to content
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

Implementation of Sidebar #36

Closed
wants to merge 18 commits into from
Closed

Implementation of Sidebar #36

wants to merge 18 commits into from

Conversation

rajesh6161
Copy link
Member

Fixes #18

Description

Sidebar implemented by taking all the considerations.

Type of change

  • New feature (non-breaking change which adds functionality pre-approved by mentors)
  • Bug fix (non-breaking change which fixes an issue)
  • Refactor
  • Addition of testcases
  • This change requires a documentation update (software upgrade on readme file)

Screenshots of change:

sidebar2
Annotation 2020-06-14 123220

Checklist:

  • My PR follows the style guidelines of this project
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have updated storybook
  • I have updated README.md
  • Added new npm packages

Code/Quality Assurance

  • My changes generate no new warnings
  • My PR currently breaks something (fix or feature that would cause existing functionality to not work as expected)
  • New and existing tests pass locally with my changes

Copy link
Contributor

@naveennvrgup naveennvrgup left a comment

Choose a reason for hiding this comment

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

Great job with PR @rajesh6161 the sidebar is awesome. I would like some changes as below:

  1. don't include unnecessary files for the PR like .vscode/c_cpp_properties.json
  2. I suggest you use the Icons from the link whenever possible https://material-ui.com/components/material-icons/
  3. src/components/SideDrawer/logo.png should be extracted to assets
  4. use class-based components and "no hooks".

I will remove unneccsary filse for you this time 😇 .

src/components/SideDrawer/SideDrawer.jsx Outdated Show resolved Hide resolved
src/components/SideDrawer/SideDrawer.jsx Outdated Show resolved Hide resolved
src/components/SideDrawer/SideDrawer.jsx Outdated Show resolved Hide resolved
src/components/GroupDetail/GroupRole.js Outdated Show resolved Hide resolved
@rajesh6161
Copy link
Member Author

Okay sir I will follow along this guideline from next time.
Thank you sir😅

@pratik0204 pratik0204 requested a review from naveennvrgup June 21, 2020 15:52
Copy link
Contributor

@naveennvrgup naveennvrgup left a comment

Choose a reason for hiding this comment

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

It could be split into multiple files. But ok for now. Thought of doing it myself but on a time crunch for now. I will do that later.

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.

Side Drawer to handle Org and group navigations
3 participants