Theme: SlidePane #248
Theme: SlidePane #248
Conversation
src/slidepane/SlidePane.ts
Outdated
| title, | ||
| v('button', { | ||
| classes: this.classes(css.close), | ||
| innerHTML: closeText, |
There was a problem hiding this comment.
I think we're generally going with text as a child over innerHTML, although I'm not sure how much it matters
There was a problem hiding this comment.
Also, I know it's a bit of a pain, but with this structure the slidepane's label text will include the close button text
src/themes/dojo/slidePane.m.css
Outdated
| .pane { | ||
| background: var(--dojo-white); | ||
| border: var(--border-thin) solid var(--border-color); | ||
| box-shadow: var(--drop-shadow); |
There was a problem hiding this comment.
I used --box-shadow-dimensions for use with custom colors for inputs, but the shadow dimensions were different for those. We should probably think of better naming 😝
src/themes/dojo/slidePane.m.css
Outdated
| } | ||
|
|
||
| .close:after { | ||
| content: '✕'; |
There was a problem hiding this comment.
doesn't need to be now, but at some point this should probably be an icon (I think it is in the designs)
src/themes/dojo/variables.css
Outdated
| --dojo-light-gray: #F4F6F7; | ||
| --dojo-dark-gray: #5C6C7C; | ||
|
|
||
| --pane-bg: var(--dojo-white); |
There was a problem hiding this comment.
I'm fine leaving out variable names that are this specific. I think if the name references a specific widget, it doesn't need to be a separate variable.
There was a problem hiding this comment.
Agreed. I use --pane-bg in a few components so far, so I'll leave this in for now.
Codecov Report
@@ Coverage Diff @@
## master #248 +/- ##
==========================================
- Coverage 98.95% 98.89% -0.06%
==========================================
Files 24 24
Lines 1429 1445 +16
Branches 413 421 +8
==========================================
+ Hits 1414 1429 +15
- Partials 15 16 +1
Continue to review full report at Codecov.
|
|
Can you please make sure that the outermost element (including the underlay) has a |
|
@tomdye: Good catch. SlidePane especially needs to use fixed classes to maintain even basic usability. I took care of this and updated the test harness expected renders. |
Type: feature
The following has been addressed in the PR:
Description:
This PR adds an initial theme for the SlidePane component.