Theme: ComboBox #258

bitpshr · 2017-07-19T14:43:31Z

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:

This PR adds an initial theme for the ComboBox component. Note that this work was based off @smhigley's TextInput theme PR.

smhigley

Some questions, and a couple focus styles. Looks good! Since it seems to be branched off textinput/textarea styles, maybe it should get merged after that PR?

smhigley · 2017-07-19T20:27:17Z

src/combobox/ComboBox.ts

@@ -401,7 +401,6 @@ export default class ComboBox extends ComboBoxBase<ComboBoxProperties> {
 				readOnly,
 				onclick: this._onArrowClick
 			}, [
-				'open combo box',


Why delete the text? It needs some sort of label, could either be this text wrapped in a visually hidden span, or an aria-label on the button

smhigley · 2017-07-19T20:28:14Z

src/themes/dojo/comboBox.m.css

+	position: absolute;
+	right: 0;
+	top: var(--border-width);
+	z-index: 1;
 }


Since combobox can have the arrow focused on separately from the input, I think this needs separate focus styles

smhigley · 2017-07-19T20:53:25Z

src/themes/dojo/comboBox.m.css

 	width: 100%;
-	padding-right: 2em;
+	z-index: 2;


We probably need some sort of consistent approach to z-indices. Maybe something like this?

10: stuff like this that appears over surrounding inline content (this, tooltips, menus)

20: block-level items that need to appear over all other normal page content (e.g. sticky navs, alerts)

30: modal overlays

40: modal content

I'm probably missing use cases, but I think it'd be good to use and document some sort of system

We could even put them in variables 😁

Agreed, good catch. I will do this as part of a separate PR, this will effect SlidePane and Dialog (at least) once they land.

smhigley · 2017-07-19T21:16:23Z

src/themes/dojo/comboBox.m.css

+	position: absolute;
+	right: calc(var(--spacing-regular) * 6);
+	top: 50%;
+	transform: translateY(-50%);


Could possibly use a focus style too

smhigley · 2017-07-19T21:17:48Z

src/themes/dojo/comboBox.m.css

-	cursor: pointer;
-	padding: var(--input-padding);
+.invalid .trigger {
+	border-left-color: color(var(--color-error) saturation(-9%) lightness(+37%));


This color (and the success variation) is getting used in more places than I thought. Do you think it's worth thinking of another variable name?

I'm fine with color-error and color-success since they provide abstraction from the underlying color. Personally I don't find myself wishing I had more abstraction, but I'm open.

tomdye · 2017-07-21T16:25:31Z

src/themes/dojo/comboBox.m.css

+	width: 100%;
+}
+
+.trigger {


is it possible to move some of this styling to a shared file? Looks like combo / select / textbox have the same styling

Totally agreed. We have #117 to track that progress, as it's more involved than just pulling out common styling. The underlying DOM is still somewhat different and would require some consolidation.

I definitely think we need to take care of that ticket asap though.

codecov · 2017-07-21T18:57:40Z

Codecov Report

Merging #258 into master will decrease coverage by 0.13%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #258      +/-   ##
==========================================
- Coverage   98.78%   98.64%   -0.14%     
==========================================
  Files          24       24              
  Lines        1478     1481       +3     
  Branches      436      439       +3     
==========================================
+ Hits         1460     1461       +1     
- Partials       18       20       +2

Impacted Files	Coverage Δ
src/combobox/ComboBox.ts	`98.28% <33.33%> (-1.14%)`	⬇️

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 8b4b841...a462537. Read the comment docs.

bitpshr · 2017-07-21T18:57:47Z

@tomdye @smhigley this should be ready for another review.

bitpshr · 2017-07-31T13:39:33Z

Bump @smhigley and @tomdye for a review. Thanks!

tomdye · 2017-07-31T14:08:09Z

src/combobox/example/index.ts

@@ -142,7 +142,9 @@ export class App extends WidgetBase<WidgetProperties> {
 	}

 	render(): DNode {
-		return v('div', [
+		return v('div', {
+			styles: { maxWidth: '256px' }


i assume this is to restrict the width of the inputs, should they not have their own max width?

Based on talking with Scott, I believe we decided no max width. It makes a certain sort of sense -- desired width can be really variable (no pun intended, mostly), and controlling input width through styles on the containing element seems like a better pattern anyway.

@bitpshr I actually wouldn't mind this added to textinput/textarea too, if you feel like it

smhigley · 2017-08-04T17:50:04Z

src/themes/dojo/comboBox.m.css

-	font-style: italic;
-	cursor: default;
+.clear:focus {
+	border-color: var(--color-highlight);


I know I'm being super nitpicky (😔), but the border isn't the most beautiful of styles. Maybe Scott would have some ideas?

Agreed. I will call this out to Scott during his sweep.

smhigley

I could do with prettier clear button focus styles, but I also think it'd be good for Scott to review all styles in action after the theme PRs are in. Either way, I'm fine with you merging this now 👍

bitpshr requested review from tomdye and smhigley July 19, 2017 14:43

smhigley suggested changes Jul 19, 2017

View reviewed changes

tomdye reviewed Jul 21, 2017

View reviewed changes

dylans modified the milestones: 2017.07, 2017.08 Jul 29, 2017

tomdye reviewed Jul 31, 2017

View reviewed changes

smhigley reviewed Aug 4, 2017

View reviewed changes

smhigley approved these changes Aug 4, 2017

View reviewed changes

smhigley and others added 14 commits August 9, 2017 07:59

textinput dojo theme

b3d0a0b

textarea theme

090e348

updated base font to font size, added transitions

3e70cc9

updated variables

594af28

Initial combobobx theme

c2c3e1a

Add focus styles

3e78f55

Update functional test selectors

e1c295e

Fix typo

cac70ec

Update functional tests

5ac3781

Use a max width on the text input example page

d40cd1c

Update text input line height

155348d

Fix styling after rebase

81abf30

Remove extraClasses

14e39e2

Remove package-lock.json

a462537

bitpshr merged commit eaee111 into dojo:master Aug 9, 2017

bitpshr deleted the combobox-theme-new branch August 9, 2017 12:29

bitpshr mentioned this pull request Aug 9, 2017

Textinput/Textarea theme #249

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theme: ComboBox #258

Theme: ComboBox #258

bitpshr commented Jul 19, 2017

smhigley left a comment

smhigley Jul 19, 2017

smhigley Jul 19, 2017

smhigley Jul 19, 2017

smhigley Jul 19, 2017

bitpshr Jul 21, 2017

smhigley Jul 19, 2017

bitpshr Jul 21, 2017

smhigley Jul 19, 2017

bitpshr Jul 21, 2017

tomdye Jul 21, 2017

bitpshr Jul 21, 2017

codecov bot commented Jul 21, 2017 •

edited

Loading

bitpshr commented Jul 21, 2017

bitpshr commented Jul 31, 2017

tomdye Jul 31, 2017

smhigley Jul 31, 2017

smhigley Aug 4, 2017

smhigley Aug 4, 2017

bitpshr Aug 9, 2017 •

edited

Loading

smhigley left a comment

Theme: ComboBox #258

Theme: ComboBox #258

Conversation

bitpshr commented Jul 19, 2017

smhigley 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

codecov bot commented Jul 21, 2017 • edited Loading

Codecov Report

bitpshr commented Jul 21, 2017

bitpshr commented Jul 31, 2017

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

bitpshr Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

smhigley left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 21, 2017 •

edited

Loading

bitpshr Aug 9, 2017 •

edited

Loading