Theme: Dialog #247
Theme: Dialog #247
Conversation
Codecov Report
@@ Coverage Diff @@
## master #247 +/- ##
==========================================
+ Coverage 98.89% 98.89% +<.01%
==========================================
Files 24 24
Lines 1448 1449 +1
Branches 423 423
==========================================
+ Hits 1432 1433 +1
Partials 16 16
Continue to review full report at Codecov.
|
smhigley
left a comment
smhigley
left a comment
There was a problem hiding this comment.
Looks good, I just commented on small stuff. Feel free to address or ignore and merge :)
src/themes/dojo/dialog.m.css
Outdated
| @import './variables.css'; | ||
|
|
||
| .main, | ||
| .main * { |
There was a problem hiding this comment.
Might as well put this under .root for consistency, I think
src/themes/dojo/dialog.m.css
Outdated
| } | ||
|
|
||
| .close:after { | ||
| content: '✕'; |
There was a problem hiding this comment.
Same with SlidePane, should probably use an icon at some point
src/themes/dojo/variables.css
Outdated
| --grid-size: 8px; | ||
| --header-font-size: 20px; | ||
| --header-line-height: 1.2; | ||
| --underlay-bg: rgba(0, 0, 0, 0.59); |
There was a problem hiding this comment.
0.6, maybe? :)
Could also use color(var(--dojo-black) alpha(60%)) for this and --drop-shadow
|
Same comment as other theme prs re. not using |
|
@tomdye: I'm not using any |
smhigley
left a comment
smhigley
left a comment
There was a problem hiding this comment.
Only comment is maybe the close button should have theme-y focus styles added. Otherwise I think it's fine to merge 👍
| title, | ||
| v('div', { id: this._titleId }, [ title ]), | ||
| closeable ? v('button', { | ||
| classes: this.classes(css.close), |
There was a problem hiding this comment.
this likely needs a closeFixed class too.
There was a problem hiding this comment.
What styling would need to be in a fixed class? With no theme (base or otherwise) the close button is still visible in the titlebar and clickable, it just has no styling.
4 participants
Type: feature
The following has been addressed in the PR:
Description:
This PR adds an initial theme for the Dialog component.