Skip to content

Tooltip shadow #1420

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed

Tooltip shadow #1420

wants to merge 9 commits into from

Conversation

jzaefferer
Copy link
Member

This is supposed to be pulled into #1384, but the GitHub UI won't let me pick Jasper's fork as the base repository.

This implements what was discussed there: "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".

Since the classes PR hasn't landed in master, yet, this just adds the ui-widget-shadow class to the tooltip element directly. We haven't discussed if we want to update ThemeRoller with new UI elements to allow configuration of box-shadow, replacing the current UI with its various settings. I tend to just dropping the whole thing. box-shadow has many properties, many of them optional, and its really easy to override in a custom CSS file. We don't have variables for every style property in theme.css already, so might as well skip this as well.

@jzaefferer
Copy link
Member Author

@jaspermdegroot @rxaviers what do you think about this? Just check the one commit I added, ignore the rest.

@rxaviers
Copy link
Member

@jzaefferer 👍, My only concern would be making sure box-shadow is supported on every browsers jQuery UI supports. http://caniuse.com/#search=box-shadow has an observation for IE8.

@jzaefferer
Copy link
Member Author

My only concern would be making sure box-shadow is supported on every browsers jQuery UI supports

I think its fine for IE8 not to display the shadow. The tooltip looks okay without it.

Can't we keep using the same variables:
[...]
Maybe rename thicknessShadow to blurShadow to make clear that it sets the blur radius and not the spread radius.

I didn't manage to match the existing properties to the box-shadow properties. Makes sense to just keep them as they are. I can update this PR accordingly. As for renaming: I guess we should rename everything or nothing. This somewhat depends on ThemeRoller, since it doesn't support multiple versions. @rxaviers does it make any difference? Is it better to keep as-is or better to rename?

@rxaviers
Copy link
Member

About renaming...

Is this change going to propagate as patch fixes for older releases too (e.g., 1.9.3, 1.10.5 and 1.11.3)? If not, supporting both names simultaneously on TR (old names for 1.9.2 and 1.10.4 and 1.11.2, but new names for 1.12.0) will require some extra not trivial code there.

Also, do we want TR to handle old -> new names mapping for people to click in the links of their saved themes and have it properly adjusted? One more extra work.

@scottgonzalez
Copy link
Member

Is this change going to propagate as patch fixes for older releases too (e.g., 1.9.3, 1.10.5 and 1.11.3)?

No.

.ui-widget-shadow was created when box-shadow wasn't available. By now,
there's no point in faking a custom shadow anymore. This removes the
only non-structural CSS from a widget-specific file.
@jzaefferer
Copy link
Member Author

Updated this to keep the previous variables. The following variables are gone, though: bgImgUrlShadow, bgShadowXPos, bgShadowYPos, bgShadowRepeat, opacityShadow, opacityFilterShadow and cornerRadiusShadow.

@jzaefferer
Copy link
Member Author

@rxaviers could you test this update with ThemeRoller?

rxaviers added a commit to jquery/download.jqueryui.com that referenced this pull request Jan 21, 2015
Temporary update jquery/jquery-ui#1420
@ d2eb0c93cfc5fce158ac2af6ca7b0f50760d9b90
@rxaviers
Copy link
Member

jquery/download.jqueryui.com#248 has been updated accordingly.

@jzaefferer
Copy link
Member Author

Replaced by #1436, which has all the details.

@jzaefferer jzaefferer closed this Jan 22, 2015
rxaviers added a commit to jquery/download.jqueryui.com that referenced this pull request Jan 22, 2015
Temporary update jquery/jquery-ui#1420
@ d2eb0c93cfc5fce158ac2af6ca7b0f50760d9b90
rxaviers added a commit to jquery/download.jqueryui.com that referenced this pull request Jan 22, 2015
Temporary update jquery/jquery-ui#1420
@ d2eb0c93cfc5fce158ac2af6ca7b0f50760d9b90

Do not use an image when using the flat texture.

Updated index.html for the 1.12.0 package.
rxaviers added a commit to jquery/download.jqueryui.com that referenced this pull request Jan 23, 2015
Temporary update jquery/jquery-ui#1420
@ d2eb0c93cfc5fce158ac2af6ca7b0f50760d9b90

Do not use an image when using the flat texture.

Updated index.html for the 1.12.0 package.
@jzaefferer jzaefferer deleted the tooltip-shadow branch March 6, 2015 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants