component(): sidenav. #24
Conversation
googlebot
added
the
cla: yes
| z-index: $z-index-drawer; | ||
| box-sizing: border-box; | ||
|
|
||
| background-color: white; |
There was a problem hiding this comment.
Color should be defined in terms of the current theme.
There was a problem hiding this comment.
Yes, that slipped through :)
There was a problem hiding this comment.
Actually, it seems the default background is rgba(0,0,0,0) which I find awkward.
hansl
force-pushed
the
md-sidenav
branch
11 times, most recently
from
193bd15
to
a4cb50f
Compare
hansl
force-pushed
the
md-sidenav
branch
2 times, most recently
from
7d88492
to
b24708f
Compare
|
@jelbourn @kara @robertmesserle Please review style and content, but not tests. |
| private start_: MdSidenav; | ||
| private end_: MdSidenav; | ||
| private right_: MdSidenav; | ||
| private left_: MdSidenav; |
There was a problem hiding this comment.
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.
Because left_ and right_ are on the right/left independently of direction, while start/end changes based on direction.
|
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? |
| describe('methods', () => { | ||
| it('should be able to open and close', (done: any) => { | ||
| let testComponent: BasicTestApp; | ||
| let fixture: ComponentFixture; |
There was a problem hiding this comment.
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. |
| /** | ||
| * Whether the sidenav is opened. We overload this because we trigger an event when it | ||
| * starts or end. | ||
| * @returns {boolean} |
There was a problem hiding this comment.
We should leave the types out of the JsDoc since they're just part of the language
|
LGTM, though tests seem to be failing |
jelbourn
added
pr: lgtm
and removed
in progress
|
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).