Great job on the Seven theme so far #614494: Give Seven a nice looking default jQuery UI theme.

I few suggested changes in this patch;

  1. Accordion styled similarly to Seven's vertical tabs. Also styled to look more "cohesive", i.e. more like one distinct widget as opposed to several separate collapsed widgets
  2. Tabs styled with a background color for the header in keeping with other Seven styling
  3. Slider selected state color and border more in keeping with Seven select color conventions
  4. Messages/Alerts icon removed (in light of Mark Boulton's talk). Also elements given consistent padding

You can see the changes here.

I posted this on the original issue cue (even though it has automatically been closed) because I was not familiar with the process. Apologies!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

edward_or’s picture

Category: feature » task
Issue tags: +JavaScript, +jQuery UI, +Seven

Status: Needs review » Needs work

The last submitted patch, jq_ui.patch, failed testing.

Jacine’s picture

Title: Tweaks to jQuerry UI Seven theme » Tweaks to jQuery UI Seven theme

Hey, thanks for the feedback and the patch! I tried to get input when doing this initially, but there were not many designers available/involved at the time. I checked out the demo, and the tabs, accordion and slider look great!

Here are my thoughts on the patch itself:

+++ b/themes/seven/jquery.ui.theme.css
@@ -13,6 +13,10 @@
+.ui-widget p {
+  margin:0;
+  padding:9px;
+}

Was this really needed? I'm not sure. It seems that regular <p> styling should be inherited here. <p> tags have margin: 1em 0; and no padding, so I'm not sure this should be affected here. In Drupal, error/status/warning messages do not come out in a <p> tag, so I'm not sure that this will be a problem, in general. However, affecting this for all widgets is probably not the best idea.

+++ b/themes/seven/jquery.ui.theme.css
@@ -79,14 +83,15 @@
 .ui-widget p .ui-icon {
-  margin: 2px 3px 0 0;
+  margin: 0;
+  display: none;
 }

No one is allowed to use icons in <p> in Seven? Error/Warning icons are not the only ones, and I would rather these be allowed to be used without module developers having to code on top of it just to get margins if they want to use any of the rest of the icons. I think the error/highlight classes should be targeted explicitly for things like this.

Also, since this code was committed, it appears the messages in Seven have been redesigned.

Powered by Dreditor.

Jacine’s picture

Also, be sure to roll the patch from the Drupal root. I think that's why it's failing the test bot.

edward_or’s picture

FileSize
17.65 KB
2.96 KB

Since this code was committed, it appears the messages in Seven have been redesigned.
I have updated the messages to match Seven message styling.

No one is allowed to use icons in <p> in Seven?
I agree, that was definitely a bad move. I have made it so that error/highlight classes are targeted explicitly.

Was this really needed? I'm not sure. It seems that regular p styling should be inherited here.

.ui-widget p {
	margin:0;
	padding:9px;
}

What does it look like for you with the regular p styling enabled? I get the bellow screenshot where the margin around the p looks fine for the highlight but does not for the error. I can't figure out why that is though. If we can make it work I think we should avoid this addition.

It is not nice to target with a p. The problem is that those "status messages" have no specific class. It would be great if there was a "message" class so that you could style them without having to use something as generic as a p tag.
Either we style the, with the .ui-widget p or we use .ui-state-highlight and override anything undesirable in cases like the date where, for instance, we want the padding to be 0px as opposed to 9px.

edward_or’s picture

Oh, and the changes are reflected on the page I linked to previously,
http://edwardoriordan.com/d7/?q=ui

edward_or’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, jq_ui.patch, failed testing.

sepgil’s picture

Hi,
while using jquery autocomplete, I noticed, that the interaction states classes aren't optimal, especially when you use jquery buttons. So I added some styles to them and also added the corner classes(copied from jquery.ui.theme.css), since people might need it.
I also changed the first comment line, since there is no file /misc/ui/ui.theme.css.

I added a before and after pictures so you could see the difference(the jquery button has an additional class from a module for size an positioning).

Jeff Burnz’s picture

If this is going to have a chance of getting in the message icons need to be reinstated and the styles changed to match core (Seven inherits cores message styling). These styles and icons were added to core for accessibility reasons - we can't just ignore that.

Jacine’s picture

Ok, but listen we need to separate these issues.

We can't do all this on one, it will go nowhere. #9 is a separate issue entirely and tries to hijack this issue.

Jeff Burnz’s picture

OK, sounds good, if we can get a re-roll based on #5, with the message styles left original - which actually look pretty accessible from a glance, we can always file a separate issue to update the message style to match core later on, if required.

sepgil’s picture

Sorry for hijacking the issue I'll open a seperate for my patch...

edward_or’s picture

Seven does not have icons for its messages as far as I can see (My point of reference is 7.0-alpha6). The only unique style for messages are

div.warning {
  color: #840;
  background: #fe6;
  border-color: #ed5;
}
div.error {
  color: #fff;
  background: #e63;
  border-color: #d52;
}

This is what I was riffing off for the jquery ui theme.


These styles and icons were added to core for accessibility reasons - we can't just ignore that.

I agree with an icons & accessibility argument. It seems to me, however, that the Seven disagrees.

edward_or’s picture

I can't figure out why my re-rolls are failing. I can't claim to be a git ninja but I am following this guide - http://drupal.org/node/707484 - to the letter and still failing.

Jeff Burnz’s picture

Arent you running HEAD? Alpha6 is way out of date now, I think you'll find message styles have been removed from Seven entirely and now inherit core styles, icons and all.

edward_or’s picture

FileSize
4.61 KB

Apologies for all the naive trampling around.

Seven does indeed now inherit its styles from core. Thanks Jeff.
This is a good thing in many ways the lest of which is that it makes it easier for a jquery ui theme to look like seven's message style.

Here is a re-roll which uses the core message style for jquery ui messages. This version is visible at http://edwardoriordan.com/d7/?q=ui

edward_or’s picture

There are a few problems with the above re-roll.

Most importantly the highlight message is not being floated to the left (screenshot attached). For some reason the alert message's icon has some styles being added with javascript which floats it and gives it a margin. The markup and styles are bellow.

<span class="ui-icon ui-icon-alert" style="float: left; margin-right: 0.3em;"></span>

Does anyone know why this happens?

My other consideration would be the size of the buttons in the dialogue box. I am a fan of big buttons but the are, in my opinion, just too large for the size of the dialogue. It is a super simple fix but I would like to hear what other people think.

Jacine’s picture

Re #17:

I love the improvements you've made to the tabs and the accordion. They look a million times better and fix odd issues that have recently appeared with the original tab code.

I am concerned about the work you've done with the errors though, and I don't think it's going to work in the current state. Looking at this code:

 .ui-state-highlight .ui-icon {
-  background-image: url(images/ui-icons-800000-256x240.png);
+  background-image:url("../../misc/message-24-warning.png");
+  background-position: 0 0;
+  width: 24px;
+  height: 24px;
 }
 .ui-state-error .ui-icon,
 .ui-state-error-text .ui-icon {
-  background-image: url(images/ui-icons-ffffff-256x240.png);
+  background-image: url(../../misc/message-24-error.png);
+  background-position: 0 0;
+  width: 24px;
+  height: 24px;
+  padding-right: 18px;
 }
-.ui-widget p .ui-icon {
-  margin: 2px 3px 0 0;
+
+/* Use core message styles (with some tweaks to reflect the different markup) */
+.ui-state-highlight, 
+.ui-state-error  {
+  border: 1px solid;
+  padding: 0 8px; /* LTR */
+}
+.ui-state-error { 
+  border-color: #ed541d;
+  color: #8c2e0b;
+  background-color: #fef5f1;
+  color: #333;
+}
+.ui-state-highlight {
+  border-color: #ed5;
+  color: #840;
+  background-color: #fffce5;
 }

The problem is that this is never really going to be able to emulate core, and it was never meant to. These classes can be used for ALL sorts of elements in jQuery UI. We have no way of even anticipating all of the possibilities. I would imagine that we could run into problems changing colors for borders, etc, but what I really worry about is the first part of this code where we are applying padding and a 1px border to these states.

Take a look at the screenshot I attached. It shows your patch on the left and HEAD on the right. I changed .ui-state-default to .ui-state-highlight and .ui-state-error on 2 of li's for the icons and on a list under the login (just to show a possible use case). That's a problem and it's really only a taste of what can happen here.

From: http://jqueryui.com/docs/Theming/API


Interaction Cues

  • .ui-state-highlight: Class to be applied to highlighted or selected elements. Applies "highlight" container styles to an element and its child text, links, and icons.
  • .ui-state-error: Class to be applied to error messaging container elements. Applies "error" container styles to an element and its child text, links, and icons.
  • .ui-state-error-text: An additional class that applies just the error text color without background. Can be used on form labels for instance. Also applies error icon color to child icons.
  • .ui-state-disabled: Applies a dimmed opacity to disabled UI elements. Meant to be added in addition to an already-styled element.
  • .ui-priority-primary: Class to be applied to a priority-1 button in situations where button hierarchy is needed. Applies bold text.
  • .ui-priority-secondary: Class to be applied to a priority-2 button in situations where button hierarchy is needed. Applies normal weight text and slight transparency to element.

Those icons are meant for system messages, and IMO they don't work anywhere else. I think that we definitely need to get the colors updated, as that never happened with the latest update to Seven, but that we still need to honor the jQuery UI functionality here as much as possible. To me, that means updating the icon images to better match the colors.

+++ b/themes/seven/jquery.ui.theme.cssundefined
@@ -268,61 +290,69 @@
 .ui-tabs .ui-tabs-nav {
-  padding: 5px 10px 4px;
-  margin: 0;
-  line-height: 20px;
-  border-bottom: solid 1px #ccc;
-  -moz-border-radius-bottomleft: 0;
-  -webkit-border-bottom-left-radius: 0;
-  -moz-border-radius-bottomright: 0;
-  -webkit-border-bottom-right-radius: 0;
+ padding: 5px 10px 4px;
+ margin: 0;
+ line-height: 20px;
+ border-bottom: solid 1px #ccc;
+ background-color: #f7f7f7;
+ -moz-border-radius-bottomleft: 0;
+ -webkit-border-bottom-left-radius: 0;
+ -moz-border-radius-bottomright: 0;
+ -webkit-border-bottom-right-radius: 0;
 }

Also, this should not happen, as only one line has actually been affected, so it makes it very hard to review. It appears that you changed the indentation? It should always be 2 spaces, and lines that you did not change should not be shown in the patch this way. The patch also needs to be run from the root of Drupal, and it looks like it wasn't, so it's being ignored.

Powered by Dreditor.

Jacine’s picture

FileSize
394.74 KB

I guess I never posted the screenshot. Oops.

Jeff Burnz’s picture

Whats the upshot here Jacine, with my earlier post regarding icons what I really meant was to restore the original jquery UI icons and message styles, rather then trying to emulate core/Seven (sorry for the confusion, i was not clear with this). I should test these properly for accessibility but strait of at a glance they don't look to bad (accessibility wise, design wise they're a bit of train wreck).

So whats the optimal solution - leave messages styles alone?

edward_or’s picture

Those icons are meant for system messages, and IMO they don't work anywhere else.
&
So whats the optimal solution - leave messages styles alone?

I think it would be nice if the error and alert messages in jquery ui were the same as used in core. I do agree, however, that the payoff here is not worth the hacking we would have to do to make it work correctly.
To my mind it is a small usability win versus a massive hit in maintainability. I vote for maintainability (and I think you all do too?)
This is the exact opposite of what I rolled in the patch however so it is time for a re-roll.

It appears that you changed the indentation?

Please excuse the indentation nonsense. As you might be able to guess this is my first time attempting this (I'm learning a whole lot). Thank you all for bearing with me and my hideous errors! I will fix this is the next patch.

Jeff Burnz’s picture

Don't sweat it, core dev is a baptism of fire at the best of times, you're doing just fine.

edward_or’s picture

Thanks Jeff. I really appreciate all the help.

Jacine’s picture

Category: task » bug
Priority: Minor » Normal

re: #21.

I think the solution is to change the colors of the icons to better match the updated message colors. This should have been done in the first place, but it must have been overlooked. That means, going to http://jqueryui.com/themeroller/ tweaking the colors, downloading the theme, and pulling out the new images to post up here. I don't think we should mess with the other stuff because we just cannot do it properly.

Accessibility changes should definitely not be part of this issue. All this CSS file means to do is change some colors and styles in the default theme, to prevent module developers from coding against the "smoothness" theme which loads by default. The core files of jQuery UI need to handle that stuff, and if it's not accessible, it's not our job rewrite jQuery UI to make it that way. I strongly believe that any attempts to improve accessibility with jQuery UI, need to be contributed to the project itself. Also, this code is not being used anywhere in core at this time, and I would imagine that future versions of jQuery UI will include accessibility improvements, and if we mess with that here, we risk breaking future updates.

Jeff Burnz’s picture

Yes, however, changing colors implies that an accessibility review is required - for color and contrast ratio's.

If we are going to be tweaking colors then I suggest using Contrast A during development to test the ratio (background to foreground) so that they pass WCAG AA (requires a ratio of 4.5:1 for "small text"). Color difference is important also to account for the various forms of color blindness.

Jacine’s picture

That's fine.

scor’s picture

If this the right issue to report a bug in the theming of the UI autocomplete? (can create separate issue otherwise)

The autocomplete suggestions are links, which are appearing blue in seven (looks great in other themes). Also the padding around them is weird, and the background is missing. You can see how the suggestions normally look like (with grey background etc) at http://docs.jquery.com/UI/Autocomplete

scor’s picture

Note that seven's override of jquery.ui.theme.css is causing some trouble in contrib with missing css images, forcing us to duplicate the jQuery UI library. Even though it is present in core, it cannot used as it is and each contrib has to package it separately. For example: #1274286: Image not found ui-bg_glass_75_dadada_1x400.png.

Jacine’s picture

Somehow this got destroyed after it was committed originally and I really think it needs to go. The current situation is very unfortunate, especially because removing this means having the smoothness theme forced on Seven (as it is on all themes), but this is just a mess and I regret even trying to get this in.

scor’s picture

@Jacine: How do we move on from here? Why can't seven's custom jquery.ui.theme.css be loaded after the default misc/ui/jquery.ui.theme.css and that way only override what's needed. This would solve my issue in fact. Tell me what to do and I'll roll a patch!

Jacine’s picture

FileSize
712.5 KB

@scor, Ok, maybe "destroyed" was too harsh a word... ;) Anyway, it was overridden because (a) it would have been a lot harder to make the changes otherwise and (b) it would have been another http request for what seems to be no reason. The changes ripped a lot of the stuff out on purpose, as part of customizing it to match Seven's branding.

The attached screenshot is what it looked like when it was committed vs. now, with the theme roller. I'm not sure what is needed here, because Seven references a custom version of these files. As you can see in the screenshot, some parts of it are messed up since the original commit. The tabs have more padding, the progress bar is bigger and the button, button set and autocomplete are new to jQuery and not accounted for in the Seven theme.

sun’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Front end

Would be nice resurrect this :)

LewisNyman’s picture

Issue summary: View changes
Status: Needs work » Closed (works as designed)

Looking at the actions in the summary, I think this issue has been superseded by the Seven style guide. I'm happy to close this now.