Added dojo theme #77

tomdye · 2017-03-17T21:22:44Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

Created copy of current styles under dojo-theme directory.
Removing any dojo specific styling from the widget css files.
Added theme switch checkbox to widget example pages.

TODO:

fix variables file
complex components
- range
- side pane
- dialog

Required Grunt dojo 2 PR for widgets.css OR m.css change.

Resolves #51

codecov · 2017-03-17T21:27:31Z

Codecov Report

Merging #77 into master will decrease coverage by 8.07%.
The diff coverage is 31.81%.

@@            Coverage Diff            @@
##           master     #77      +/-   ##
=========================================
- Coverage   99.78%   91.7%   -8.08%     
=========================================
  Files          18      30      +12     
  Lines         920    1001      +81     
  Branches      274     278       +4     
=========================================
  Hits          918     918              
- Misses          0      78      +78     
- Partials        2       5       +3

Impacted Files	Coverage Δ
src/tabpane/TabPane.ts	`98.91% <0%> (-1.09%)`	⬇️
src/themes/dojo/theme.ts	`0% <0%> (ø)`
src/slidepane/SlidePane.ts	`98.57% <100%> (ø)`	⬆️
src/select/Select.ts	`99.35% <100%> (ø)`	⬆️
src/button/Button.ts	`100% <100%> (ø)`	⬆️
src/combobox/ResultMenu.ts	`100% <100%> (ø)`	⬆️
src/combobox/ComboBox.ts	`98.79% <33.33%> (-1.21%)`	⬇️
_build/themes/dojo/slidePane.m.css.js	`0% <0%> (ø)`
_build/themes/dojo/textinput.m.css.js	`0% <0%> (ø)`
_build/themes/dojo/slider.m.css.js	`0% <0%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ae7f14...32f3f5c. Read the comment docs.

bitpshr

Some questions and comments. Tests should probably be touched up, coverage decreased. Also, I pulled down your branch and the theme didn't work. I ran npm install && grunt on a clean clone. Example pages loaded fine, but the check box on each page did nothing (and didn't produce any JS errors.) It looked like only the structural classes were applied to elements, not theme classes.

bitpshr · 2017-04-03T14:04:38Z

src/button/example/index.ts

@@ -4,9 +4,19 @@ import { StatefulMixin } from '@dojo/widget-core/mixins/Stateful';
 import { ProjectorMixin } from '@dojo/widget-core/mixins/Projector';
 import { w, v } from '@dojo/widget-core/d';
 import Button, { ButtonType } from '../../button/Button';
+import dojo from '../../themes/dojo/theme';


This import may be better named something like dojoTheme to avoid confusion. Applies to all example pages.

bitpshr · 2017-04-03T14:05:18Z

src/button/example/index.ts

+	private _theme: {};
+
+	themeChange(event: Event) {
+		console.log('called');


This log statement can be removed. Applies to all example pages.

bitpshr · 2017-04-03T14:11:34Z

src/combobox/styles/comboBox.m.css

@@ -4,7 +4,7 @@
 	position: relative;
 	display: inline-block;
 	box-sizing: border-box;
-	width: var(--form-field-width);
+	width: 200px;


Should we still use some variables for structural styles just so everything looks consistent even when not themed? It would also help maintainability.

fair comment

I think the initial sweep of this was just to remove it all, we can do a second sweep to add these in after. This PR keeps getting out of date and is a pain to maintain.

bitpshr · 2017-04-03T14:14:20Z

src/radio/styles/radio.m.css

-.readonly .inputWrapper::before {
-	border-color: var(--input-border-color);
-}
+.input {}


Just clarifying, these empty rulesets have to be defined so they can be themed, right?

yes, correct

bitpshr · 2017-04-03T14:15:46Z

src/textarea/example/index.ts

+		this.invalidate();
+	}
+
+	onChange(event: Event) {


Is this function ever used?

bitpshr · 2017-04-03T14:17:04Z

src/textinput/example/index.ts

+		this.invalidate();
+	}
+
+	onChange(event: Event) {


Is this function ever used?

tomdye · 2017-04-07T12:40:21Z

@bitpshr as in the comment it requires the linked grunt-dojo2 pr

tomdye · 2017-04-21T12:21:15Z

@bitpshr this is ready for re-review.
I think we need to make our mind up as to if we drop the dojo-theme until it's done? ie. I don't think we should ship it with beta 2 unless it's upto scratch.

dylans · 2017-04-21T12:32:25Z

I think we should make it a priority to get it up to scratch for beta2 :)

tomdye · 2017-04-21T13:13:49Z

@dylans that is what I was getting at.
This is part one of the theme work. Next stage is to sanitise class names / base theme and then write the dojo theme. We will certainly need @itorrey input to achieve the last part.

bitpshr

A few log statements and one note about making a ticket before landing, but this looks fine. We should ideally not go down in our coverage though...

bitpshr · 2017-04-21T16:09:44Z

src/combobox/styles/comboBox.m.css

@@ -31,27 +31,26 @@
 .results {
 	position: absolute;
 	width: 100%;
-	max-height: var(--select-menu-height);
+	max-height: 400px;


Re: the comment above, we should make a ticket to variable-ize the base theme for maintainability and consistency before landing this so we don't lose track of that work.

bitpshr · 2017-04-21T16:10:29Z

src/dialog/example/index.ts

+	private _theme: {};
+
+	themeChange(event: Event) {
+		console.log('called');


console.log

bitpshr · 2017-04-21T16:11:15Z

src/slidepane/example/index.ts

+	private _theme: {};
+
+	themeChange(event: Event) {
+		console.log('called');


console.log

rishson · 2017-04-21T16:34:51Z

Refs #122 #123 #124

tomdye · 2017-04-21T16:35:44Z

@rishson the above issues I just created are for the next part of this work. this PR enables those issues to be addressed.

rishson · 2017-04-21T16:37:04Z

@tomdye indeed they are, and so when I can't remember what they were, I'll dig out this theme PR and have nice links to them.

dylans added this to the 2017.03 milestone Mar 18, 2017

rishson added the beta2 label Mar 30, 2017

tomdye force-pushed the theme branch from 8ea1581 to 4647cc3 Compare March 31, 2017 08:16

dylans modified the milestones: 2017.03, 2017.04 Apr 2, 2017

bitpshr suggested changes Apr 3, 2017

View reviewed changes

bitpshr mentioned this pull request Apr 3, 2017

Select 05 #94

Merged

3 tasks

tomdye force-pushed the theme branch from 791be1c to 2efbaf1 Compare April 7, 2017 12:33

tomdye added 6 commits April 20, 2017 15:13

added theme / switch for standard widget inputs

bd26ad8

fixed conflicts

d26bd4e

adding theme split

2089339

last few;

5ac56af

added tabpane theme

85400b9

pr review

06159d4

tomdye force-pushed the theme branch from 591b99c to 06159d4 Compare April 20, 2017 14:16

adding select theme file

cfb9549

tomdye requested a review from agubler April 21, 2017 10:43

bitpshr approved these changes Apr 21, 2017

View reviewed changes

removed some logs

32f3f5c

tomdye merged commit 185b54e into dojo:master Apr 21, 2017

tomdye mentioned this pull request Jan 30, 2020

Create Material Theme #1048

Closed

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added dojo theme #77

Added dojo theme #77

tomdye commented Mar 17, 2017 •

edited

Loading

codecov bot commented Mar 17, 2017 •

edited

Loading

bitpshr left a comment

bitpshr Apr 3, 2017

tomdye Apr 7, 2017

bitpshr Apr 3, 2017

tomdye Apr 7, 2017

bitpshr Apr 3, 2017

tomdye Apr 7, 2017

tomdye Apr 7, 2017

bitpshr Apr 3, 2017

tomdye Apr 7, 2017

bitpshr Apr 3, 2017

bitpshr Apr 3, 2017

tomdye commented Apr 7, 2017

tomdye commented Apr 21, 2017

dylans commented Apr 21, 2017

tomdye commented Apr 21, 2017

bitpshr left a comment •

edited

Loading

bitpshr Apr 21, 2017

tomdye Apr 21, 2017

bitpshr Apr 21, 2017

bitpshr Apr 21, 2017

rishson commented Apr 21, 2017

tomdye commented Apr 21, 2017 •

edited

Loading

rishson commented Apr 21, 2017

Added dojo theme #77

Added dojo theme #77

Conversation

tomdye commented Mar 17, 2017 • edited Loading

codecov bot commented Mar 17, 2017 • edited Loading

Codecov Report

bitpshr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdye commented Apr 7, 2017

tomdye commented Apr 21, 2017

dylans commented Apr 21, 2017

tomdye commented Apr 21, 2017

bitpshr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishson commented Apr 21, 2017

tomdye commented Apr 21, 2017 • edited Loading

rishson commented Apr 21, 2017

tomdye commented Mar 17, 2017 •

edited

Loading

codecov bot commented Mar 17, 2017 •

edited

Loading

bitpshr left a comment •

edited

Loading

tomdye commented Apr 21, 2017 •

edited

Loading