Theme #1384

jaspermdegroot · 2014-11-07T09:58:32Z

I wasn't sure if switching to shorthand for hex color codes would cause problems with the ThemeRoller so I didn't follow our new CSS style guide.

jzaefferer · 2014-11-07T11:08:04Z

Nice, I think this looks great. The new active color has much more contrast then before, which used to be too subtle.

A few nitpicks:

the "today" color on datepicker looks, a greenish yellow
the box-shadow on tooltip blends in with the border color, that needs some adjustment. Maybe just remove the box-shadow entirely
for nested menus, the parent menu item of a focused submenu gets the blue active color. Now that the color is less subtle, this looks like a bad idea. Not an issue of this PR itself, just something it highlights. Would like to hear what others think about this.

That's all I found when checking all demos and visual tests in Chrome. Can check more browsers later.

scottgonzalez · 2014-11-07T12:45:38Z

themes/base/theme.css

@@ -15,7 +15,7 @@
 /* Component containers
 ----------------------------------*/
 .ui-widget {
-	font-family: Verdana,Arial,sans-serif/*{ffDefault}*/;
+	font-family: Arial, Helvetica, sans-serif/*{ffDefault}*//*{ffDefault}*/;


This will break TR. Spaces are not allowed in values.

scottgonzalez · 2014-11-07T12:48:22Z

I got about ten lines into the review and stopped because this is going to destroy ThemeRoller.

As for @jzaefferer's comment about menu items, highlighting the parent is important. I'm not sure why you think this is a bad idea, just look at a native menu.

jzaefferer · 2014-11-07T12:54:53Z

just look at a native menu.

Looking at the menu bar in OSX, both "active" and focused items are displayed in blue. Instead we show a light gray for focused items and the active blue only on a parent menu item with a focused submenu item.

jzaefferer · 2014-11-07T12:57:30Z

I got about ten lines into the review and stopped because this is going to destroy ThemeRoller.

In less fatal terms, we need to keep all the placeholders, which affects only the background rules. Restoring those shouldn't be a big deal.

scottgonzalez · 2014-11-07T13:13:42Z

Looking at the menu bar in OSX, both "active" and focused items are displayed in blue. Instead we show a light gray for focused items and the active blue only on a parent menu item with a focused submenu item.

Got it. I didn't get to the point of viewing the theme. We should probably not differentiate between active and focused for menus. I'm pretty sure this has come up a few times and it's always been confusing. Treating the two states the same would simplify it and make it look better.

jzaefferer · 2014-11-07T13:47:51Z

I created a ticket for the menu issue: http://bugs.jqueryui.com/ticket/10692. This PR shouldn't get blocked by that.

jaspermdegroot · 2014-11-07T15:41:29Z

@jzaefferer @scottgonzalez

the "today" color on datepicker looks, a greenish yellow

It is greenish yellow :) I thought that would make it look a bit more modern. Do you want it to be more yellow and less green?
Also note that I changed the background color of the error state. I didn't notice an example of that in the demos so you might want to add the class somewhere to check it out.

the box-shadow on tooltip blends in with the border color, that needs some adjustment. Maybe just remove the box-shadow entirely

I made the shadow a bit lighter. What do you think of that?

This will break TR. Spaces are not allowed in values.

I removed the spaces from the font-family values.

In less fatal terms, we need to keep all the placeholders, which affects only the background rules.

I added the comments/placeholders back. Will the ThemeRoller set the background position and repeat values, despite the fact that you can't adjust those values in the ThemeRoller?

scottgonzalez · 2014-11-07T15:46:59Z

Will the ThemeRoller set the background position and repeat values, despite the fact that you can't adjust those values in the ThemeRoller?

ThemeRoller is just a very simple token replacer. Anywhere that a /*{name}*/ token exists, the token and all preceding characters up to a space will be replaced with the value.

jaspermdegroot · 2014-11-07T15:57:44Z

ThemeRoller is just a very simple token replacer. Anywhere that a /{name}/ token exists, the token and all preceding characters up to a space will be replaced with the value.

I know, but what is "the value" if you can't set it anywhere in the ThemeRoller? Unless I missed something you can only set the background image, not the position and repeat values.

jzaefferer · 2014-11-07T16:00:11Z

Will the ThemeRoller set the background position and repeat values, despite the fact that you can't adjust those values in the ThemeRoller?
ThemeRoller is just a very simple token replacer. Anywhere that a /{name}/ token exists, the token and all preceding characters up to a space will be replaced with the value.

I think the point was that there are multiple placeholders that ThemeRoller has no UI for, so in practice the value of those placeholders will never be changed by ThemeRoller. Between new CSS framework and merging or rewriting ThemeRollers, it seems fine to just drop unused placeholders now.

It is greenish yellow

Sorry, looks like I didn't quite finish my comment there. It is greenish yellow, yes, and in my eyes it looks really bad. Sticks out like a "sore thumb" (not sure if that is a good metaphor here...).

tests/visual/tooltip/tooltip.html has both ui-state-error and ui-state-highlight (on the right side of the page. The error state looks good, highlight doesn't.

I made the shadow a bit lighter. What do you think of that?

Looks good now.

scottgonzalez · 2014-11-07T16:07:45Z

The error state looks good

I think this looks pretty bad, due to the lack of contrast. Even with large text (18pt), this fails WCAG's minimum contrast requirements.

jzaefferer · 2014-11-07T16:23:45Z

Ah yes, contrast is bad. I only glanced at the background color. Text color should we different to provide more contrast.

jaspermdegroot · 2014-11-08T14:55:05Z

I made some changes. Let me know what you think.

it seems fine to just drop unused placeholders now

The question is where/how do we set background position (50% 50%) and repeat (repeat-x) values for custom themes that do use background images. The ThemeRoller will set the background image value because the placeholder is still there, but it won't set position and repeat values because there is no UI for it (keeping or dropping the placeholder won't make a difference). Or am I wrong?

jzaefferer · 2014-11-18T15:22:13Z

I made some changes. Let me know what you think.

Looks good to me.

The question is where/how do we set background position (50% 50%) and repeat (repeat-x) values for custom themes that do use background images.

Is there a way to keep those defaults even without a url property? Or an empty url()?

jaspermdegroot · 2014-11-18T17:43:27Z

I took a closer look at how the jQuery UI ThemeRoller works and noticed that I was wrong. The TR will set the position and repeat values because they are hardcoded in /lib/themeroller.js. I will add those placeholders back.

Update: I already did add those placeholders back in commit 44584e8

jzaefferer · 2014-11-19T16:03:52Z

Nice, so this should be ready for TR. Which we should still test. @rxaviers what's a good way to test this new theme within TR?

rxaviers · 2014-11-19T16:46:54Z

Give it a name and add it to TR as a new named theme? I'm assuming this new theme is compatible with the previous one (in other words, one would be able to create this theme by using TR).

jzaefferer · 2014-11-19T17:14:35Z

This replaces the base ("default") theme, so that's what we need to test. Creating a separate theme doesn't seem like a good test. Also I don't know if TR could generate this theme.

rxaviers · 2014-11-19T19:09:38Z

What's the plan to support the previous (current) themes after this new base theme lands since they are incompatible? I'm assuming they are incompatible given your reply.

rxaviers · 2014-12-11T16:16:17Z

Jörn and I have talked and giving the fact the variables are still the same we'll update TR and give it a try. I'll update here when it's done.

jzaefferer · 2015-01-06T17:28:43Z

@rxaviers can you look into this asap? We're getting closer to 1.12.

rxaviers · 2015-01-06T17:30:49Z

Sure, it will be my priority #1 for tomorrow.

rxaviers · 2015-01-07T14:54:22Z

themes/base/tooltip.css

-	-webkit-box-shadow: 0 0 5px #aaa;
-	box-shadow: 0 0 5px #aaa;
+	-webkit-box-shadow: 0 0 5px #ddd;
+	box-shadow: 0 0 5px #ddd;


What's the motivation of this change? I'm asking, because it doesn't feel natural to me having a structural stylesheet changed for a theme adjustment.

Perhaps the question must be should we consider moving this into theme.css? Tooltip.css is the only structural stylesheet including a color as far as I can tell.

In today's UI meeting, it was decided to "Update tooltip to use ui-widget-shadow class (via classes option, once that lands), move box-shadow definition into theme.css/.ui-widget-shadow, update TR to demo that instead of the weird fake shadow we have currently"

@jaspermdegroot could you implement this? Let me know if you have questions.

@jzaefferer - Sure, I will make some time for this next week.

@jaspermdegroot Did you get a chance to work on this?

@scottgonzalez - I didn't because @jzaefferer already created PR #1420. I think that PR, together with jquery/download.jqueryui.com#248, covers everything that needs to be done. Or did I miss something?

Ah, right. Nevermind. I was just looking over this and saw the last comment saying you were going to make a change last week.

rxaviers · 2015-01-07T19:03:00Z

Except for the change in tooltip.css, everything else have been made onto theme.css and are bound directly to a variable. It means TR can generate it just fine for 1.12 and 1.11 (and earlier versions - need to check). Note it doesn't necessarily mean user will be able to use the TR interface to come up with the above theme. Because, a few variables changed here have no corresponding UI (i.e., bg*Url, bg*XPos and bg*YPos).

Currently, the default theme of a custom downloaded package is UI Lightness. The theme that reflects the base theme is called Smoothness. @jzaefferer and I have discussed and brought to UI's meeting today the inconsistency of having UI Lightness as the default theme of a custom download, but using the base (aka smoothness) theme as default on the repo and demos (including the one in the api docs).

The accepted suggestion was to keep Smoothness and UI Lightness in the gallery, while to make the new proposed theme the default everywhere available as “Base”.

jaspermdegroot · 2015-01-08T13:55:38Z

@rxaviers

Note it doesn't necessarily mean user will be able to use the TR interface to come up with the above theme. Because, a few variables changed here have no corresponding UI (i.e., bg_Url, bg_XPos and bg*YPos).

You can select a background image in the ThemeRoller so I am not sure what you mean by no UI for bg Url.
Regarding bg XPos and bg YPos: The user doesn't need to be able to change the values for those variables so it's fine that there is no UI for it. Their values are hardcoded (see my comment). No changes here compared to current situation.
We only need to make sure that the background-position rule is only included in the theme CSS when the user sets a background image, not when s/he doesn't (because this new default theme doesn't use bg images). If I remember correctly the current script in /lib/themeroller.js will take care of this.

rxaviers · 2015-01-08T13:57:28Z

@jaspermdegroot you cannot select none. You must always pick a texture via TR UI.

jaspermdegroot · 2015-01-08T14:11:48Z

@rxaviers - Do you think it's easy to add "none" to the texture control?

rxaviers · 2015-01-08T14:12:55Z

Not hard for sure.

Keep "Smoothness" and "UI Lightness" in the gallery, while the new theme will be available as "Base" and it will be the default. Ref jquery/jquery-ui#1384

rxaviers · 2015-01-08T19:05:00Z

@jaspermdegroot, @jzaefferer and @scottgonzalez feel free to review jquery/download.jqueryui.com#248. (a) there is a picture of the ThemeRoller displaying "Base", (b) there are jQuery UI 1.9.2, 1.10.4, 1.11.2, 1.12.0-pre packages available for download using "Base", (c) there are steps to reproduce DB/TR locally using "Base". Just let me know in any questions.

kborchers · 2015-01-12T16:19:51Z

I thought I had seen at one point a mockup of the new theme without the rounded corners. Can we modernize this new theme a bit more by removing them rather than just slightly changing the radius?

arschmitz · 2015-01-12T16:24:40Z

Personal preference I would not remove the radius entirely, but I would make it a lot smaller.

@kborchers I think the mock up you saw used the old demos font size and since the radius is in em it would have therefor been a smaller radius.

kborchers · 2015-01-12T16:33:47Z

Yeah, I guess maybe a smaller radius would be fine too. Another look does show most themes still have some radius but they are much smaller so I would be fine with that too. Maybe 3px is ok. It could be better use of shadows in other framework themes that I like more as well. I don't know, I'm not a designer and just throwing in my 2 cents so take it or leave it. 😄

jaspermdegroot · 2015-01-12T22:13:05Z

@kborchers @arschmitz - I am not sure what you guys are looking at. I did already reduce the border-radius to 3px in this PR.

And if someone doesn't want rounded corners at all, he/she can remove the corner class with the new classes option I suppose.

rxaviers · 2015-01-21T12:18:48Z

Is this theme supposed to work on 1.11.x, 1.10.x or 1.9.x too without any change? Or is this only intended to work on 1.12.x and up?

scottgonzalez · 2015-01-21T14:16:15Z

I'd say it only matters for 1.12+, but I would assume that it would work on all versions.

rxaviers · 2015-01-21T14:25:19Z

I'm concerned about having download builder (which serves 1.11.x, 1.10.x and 1.9.x) to distribute this theme by default giving it hasn't been tested on previous versions. So, I'm wondering if @jaspermdegroot (that created the theme) or anyone that is reviewing it could ensure it can be used just fine without any modification to the previous releases.

scottgonzalez · 2015-01-21T14:45:36Z

Isn't this exactly what ThemeRoller does? I don't see any reason this wouldn't work.

rxaviers · 2015-01-21T15:07:45Z

I made a wrong assumption (discussed via IRC), nevermind.

jzaefferer · 2015-01-21T17:24:54Z

I'm closing this in favour of #1420 to prevent further confusion.

Keep "Smoothness" and "UI Lightness" in the gallery, while the new theme will be available as "Base" and it will be the default. Ref jquery/jquery-ui#1384

jaspermdegroot added 3 commits November 7, 2014 10:43

Theme: Tweaked the default theme

6f9a6f9

Demos: Use same font-family for demos

7efaa39

Theme: Removed unused images

6e7b527

scottgonzalez reviewed Nov 7, 2014
View reviewed changes

jaspermdegroot added 2 commits November 7, 2014 15:49

Theme: Restored comments for the ThemeRoller

44584e8

Theme: Lighter box-shadow for tooltip

2495249

jaspermdegroot added 3 commits November 8, 2014 13:17

Theme: Changed highlight and error styles

3f8e457

Theme: Changed default and hover interaction states font color

4c3013a

Theme: Changed icon colors

1a10f10

rxaviers reviewed Jan 7, 2015
View reviewed changes

rxaviers mentioned this pull request Jan 8, 2015

Theme: Add new theme called "Base" jquery/download.jqueryui.com#248

Closed

1 task

jzaefferer mentioned this pull request Jan 12, 2015

Tooltip shadow #1420

Closed

jzaefferer mentioned this pull request Jan 19, 2015

Menu: Use ui-state-active consistently #1429

Merged

jaspermdegroot mentioned this pull request Jan 21, 2015

Update ThemeRoller UI to reflect changes in default theme jquery/download.jqueryui.com#249

Open

jzaefferer closed this Jan 21, 2015

jzaefferer mentioned this pull request Jan 22, 2015

Theme update #1436

Closed

rxaviers mentioned this pull request Jul 14, 2015

Packer: Support building 1.12.0 package jquery/download.jqueryui.com#256

Closed

9 tasks

Theme #1384

Theme #1384

Conversation

jaspermdegroot commented Nov 7, 2014

jzaefferer commented Nov 7, 2014

Choose a reason for hiding this comment

scottgonzalez commented Nov 7, 2014

jzaefferer commented Nov 7, 2014

jzaefferer commented Nov 7, 2014

scottgonzalez commented Nov 7, 2014

jzaefferer commented Nov 7, 2014

jaspermdegroot commented Nov 7, 2014

scottgonzalez commented Nov 7, 2014

jaspermdegroot commented Nov 7, 2014

jzaefferer commented Nov 7, 2014

scottgonzalez commented Nov 7, 2014

jzaefferer commented Nov 7, 2014

jaspermdegroot commented Nov 8, 2014

jzaefferer commented Nov 18, 2014

jaspermdegroot commented Nov 18, 2014

jzaefferer commented Nov 19, 2014

rxaviers commented Nov 19, 2014

jzaefferer commented Nov 19, 2014

rxaviers commented Nov 19, 2014

rxaviers commented Dec 11, 2014

jzaefferer commented Jan 6, 2015

rxaviers commented Jan 6, 2015

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

rxaviers commented Jan 7, 2015

jaspermdegroot commented Jan 8, 2015

rxaviers commented Jan 8, 2015

jaspermdegroot commented Jan 8, 2015

rxaviers commented Jan 8, 2015

rxaviers commented Jan 8, 2015

kborchers commented Jan 12, 2015

arschmitz commented Jan 12, 2015

kborchers commented Jan 12, 2015

jaspermdegroot commented Jan 12, 2015

rxaviers commented Jan 21, 2015

scottgonzalez commented Jan 21, 2015

rxaviers commented Jan 21, 2015

scottgonzalez commented Jan 21, 2015

rxaviers commented Jan 21, 2015

jzaefferer commented Jan 21, 2015