- 
                Notifications
    You must be signed in to change notification settings 
- Fork 6.8k
component(): sidenav. #24
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
Conversation
        
          
                src/components/sidenav/drawer.scss
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color should be defined in terms of the current theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that slipped through :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it seems the default background is rgba(0,0,0,0) which I find awkward.
193bd15    to
    a4cb50f      
    Compare
  
    7d88492    to
    b24708f      
    Compare
  
    | @jelbourn @kara @robertmesserle Please review style and content, but not tests. | 
        
          
                src/components/sidenav/sidenav.ts
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are there left_ and right_ properties? If they're just aliases, I think that's confusing and prone to errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because left_ and right_ are on the right/left independently of direction, while start/end changes based on direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add explanation?
| cc @juliemr would you be able to take a quick peek at the unit tests here to make sure we're setting ourselves up with good practices? | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you gain from declaring fixture out here? Why not just do .then(fixture: ComponentFixture) in each of the tests and save some lines?
| @juliemr I am chaining all the promises. In order to do that I'd need to return the fixture for every executor function. | 
| ah, gotcha. OK, test looks good to me. | 
        
          
                src/components/sidenav/sidenav.ts
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should leave the types out of the JsDoc since they're just part of the language
| LGTM, though tests seem to be failing | 
| It's weird that we have methods that returns something with a  Tests are flaky. I'll rerun them until they pass. :/ | 
| This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. | 
@jelbourn @robertmesserle @kara
Design:
Todo:
containerattribute on the drawers.display: nonewhen fully closed (after transition end).