Skip to content

Conversation

@iFlameing
Copy link
Collaborator

Regarding issue #225

@helfi92 can you make some comment on this. The only problem is that when I am making drawerOpen true I am able to show the drawer in desktop mode but I am not able to remove it when I am clicking on any sidebar list. can you take a look or suggest me anything after looking this?

@iFlameing iFlameing requested a review from a team as a code owner February 18, 2019 18:30
Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

The only problem is that when I am making drawerOpen true I am able to show the drawer in desktop mode but I am not able to remove it when I am clicking on any sidebar list.

I see two issues:

  1. There seems to be 3 Drawer component being used in Dashboard/index.jsx. We only need to have 1. You can remove the ones that have the Hidden component right above it.
  2. The Drawer component is missing the onClose and the proper variant prop. That's why it's not being collapsing when clicking on a sidebar element. For a list of the Drawer props, you can look at the material docs. Hint: the variant should not be permanent.

@iFlameing
Copy link
Collaborator Author

@helfi92 thanks, I think it is ready for review. can you review this? :)

@helfi92 helfi92 self-requested a review February 19, 2019 12:54
Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

Good progress :) It turns out we need to handle the documentation site (anything under /docs) a bit differently. The drawer is pretty important for docs so we should keep it there. I added some changes in this commit. Please apply them and try to see what I did there. Feel free to copy paste the following two files: src/components/Dashboard/index.jsx src/components/NotFound/index.jsx.

@iFlameing
Copy link
Collaborator Author

@helfi92 I modify my code as you said and saw that previously for Documentation also we have a temporary variant drawer. But with these changes, we have a permanent drawer as it is important for a newcomer. But there is a problem in mobile view which I didn't like i.e( when we are clicking on the DocsSidebarList, drawer is still open and we have to click on the backdrop to close it.) What do you think and also what should I do now after these changes?

@helfi92
Copy link
Contributor

helfi92 commented Feb 19, 2019

You can try to figure it out. I'm also ok merging after you add the requested changes then we could fix the issue you pointed out in a different PR.

@iFlameing
Copy link
Collaborator Author

@helfi92 I pushed the commit which is required. you can merge this now :) thanks for the help.

Copy link
Contributor

@helfi92 helfi92 left a comment

Choose a reason for hiding this comment

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

😍

@helfi92 helfi92 merged commit 2b77a7b into taskcluster:master Feb 19, 2019
imbstack pushed a commit that referenced this pull request May 5, 2020
imbstack pushed a commit that referenced this pull request May 7, 2020
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