Skip to content
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

Theme: Revert three theme commits that are causing styling issues #1983

Merged
merged 3 commits into from Sep 25, 2021

Conversation

mgol
Copy link
Member

@mgol mgol commented Sep 24, 2021

This reverts three commits:

which caused styling issues when compared to UI 1.12.1.

This unfixes a few issues:

However, old & known issues are better than new & unknown ones, especially with our current very limited resources.

@mgol mgol added this to the 1.13.0 milestone Sep 24, 2021
@mgol mgol self-assigned this Sep 24, 2021
@fnagel
Copy link
Member

fnagel commented Sep 24, 2021

Would be amazing if @jzaefferer could help a little here :-D

Copy link
Member

@fnagel fnagel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still feel a little sick when thinking about reverting fixes for bugs but I see that we definitely need to fix the unusable themes.

At least we should create a follow up ticket with all information we have. I can do this after my vacation. Maybe we'll find a solution sometime.

Copy link
Member

@fnagel fnagel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhhh, I've tested this a little more and I see those issues: 1) a little glitch with the icon in radio / checkboxes (1.13 only) and 2) a fully broken checkbox in ui lightness theme (1.12 only) and 3) an ugly design glitch with radios in ui lightness theme (1.12 only)

Don't we get the broken checkbox in UI Lightness like in 1.12 when applying this PR?

@mgol
Copy link
Member Author

mgol commented Sep 24, 2021

@fnagel

At least we should create a follow up ticket with all information we have. I can do this after my vacation. Maybe we'll find a solution sometime.

Cool, I'll leave that for you to create then.

I did spend some time trying to fix the issues without reverting the changes but those commit are pretty substantial; they drastically change the ideas around which colors to use when and I wasn't able to see a quick way to resolve the issues. It might be that way bigger changes would be necessary, maybe even including changes in theme colors.

It's possible there's an easier & simpler way to fix the affected issues; that would be preferable.

Mhhh, I've tested this a little more and I see those issues: 1) a little glitch with the icon in radio / checkboxes (1.13 only)

Is that the visible dash that should be hidden? I'll prepare a separate PR for that one.

  1. a fully broken checkbox in ui lightness theme (1.12 only)
  2. an ugly design glitch with radios in ui lightness theme (1.12 only)

What's the glitch with radios?

Don't we get the broken checkbox in UI Lightness like in 1.12 when applying this PR?

We do. The last sentence of the PR description captures my thoughts around it:

However, old & known issues are better than new & unknown ones, especially with our current very limited resources.

@fnagel
Copy link
Member

fnagel commented Sep 24, 2021

I did spend some time trying to fix the issues without reverting the changes but those commit are pretty substantial; they drastically change the ideas around which colors to use when and I wasn't able to see a quick way to resolve the issues. It might be that way bigger changes would be necessary, maybe even including changes in theme colors.

Mhh ok. The issue looks so small and fixable but I had no time digging into it yet. And I remember you already said something like its a tough one. Sorry for the duplicate question ^^

Is that the visible dash that should be hidden?

Yes

What's the glitch with radios?

In 12.1 it looks like this (the unchecked are more visible then the active one):
grafik

and like this in 1.13:
grafik

Anyway, this one is not a big problem IMO.

We do. The last sentence of the PR description captures my thoughts around it:
However, old & known issues are better than new & unknown ones, especially with our current very limited resources.

Hehe, yes, that sounds valid. I guess we don't have much options here :-/

@fnagel
Copy link
Member

fnagel commented Sep 24, 2021

Maybe it's an option to add a workaround (e.g. some exrta CSS) to the ui-lighness theme to fix the 1.12 checkbox issue?

@mgol
Copy link
Member Author

mgol commented Sep 24, 2021

Maybe it's an option to add a workaround (e.g. some exrta CSS) to the ui-lighness theme to fix the 1.12 checkbox issue?

Themes are produced from https://github.com/jquery/jquery-ui/blob/main/themes/base/theme.css, mostly by changing colors to variables declared in comments like /*{borderColorContent}*/; those variables are defined in https://github.com/jquery/download.jqueryui.com/blob/main/lib/themeroller-themegallery.js for each theme. I don't see a way to add a special rule for a single theme, it's all driven by those theme variables.

All of the themes should be possible to generate using the Themeroller just by entering proper variable values; any special rule would violate such a constraint.

@mgol
Copy link
Member Author

mgol commented Sep 24, 2021

I submitted a fix for visible dashes in blank icons in #1987.

@mgol
Copy link
Member Author

mgol commented Sep 24, 2021

@fnagel the main issue is that two of the three mentioned commits change icon background styles to use border set to borderColorActive and background-color to bgColorContent - but in some themes, including the base one, those colors are very close to each other. This can only be fixed by changing the color definitions - which I want to avoid as that's pretty involved and I'm not a graphic designer - or by using different colors and then the question is which ones and that needs to be tested in all the 25 themes.

The second issue is the new .ui-state-default styles that override some icon styles, making them light grey instead of dark grey in the demo, it also causes many other weird differences.

@fnagel
Copy link
Member

fnagel commented Sep 25, 2021

I don't see a way to add a special rule for a single theme, it's all driven by those theme variables.
All of the themes should be possible to generate using the Themeroller just by entering proper variable values; any special rule would violate such a constraint.

I already assumed it's not really possible to do this easily and you're right: it would not be a clean solution. More like a hack :-/

@fnagel
Copy link
Member

fnagel commented Sep 25, 2021

I submitted a fix for visible dashes in blank icons in #1987.

This looks pretty good to me. Any easy way to test this locally with another theme?

@mgol
Copy link
Member Author

mgol commented Sep 25, 2021

@fnagel

This looks pretty good to me. Any easy way to test this locally with another theme?

There's a way, albeit a bit hacky.

First, you need to make sure the commit you want to test is on the origin, not on your fork. You may need to temporarily push a branch to the main jQuery UI repo. Then, copy the full commit ID of a commit you want to test.

Next, make sure you have the latest version of https://github.com/jquery/download.jqueryui.com. Then you need to open config.json and change the version field from 1.13.0-rc.2 to the commit ID you copied before. Delete the jquery-ui folder in the repo. I found that having both a recent commit ID in one entry and 1.13.0-rc.2 in a separate one doesn't work; the RC.2 version is served for the new entry as well. You can download the RC.2 themes from the official site for comparison since you won't have them in your local Download Builder due to this quirk.

Afterwards, run the following commands:

npm install
grunt prepare
node server.js --console

and open http://localhost:8080 in your browser. Click on the Download Builder link and you'll have a local version of the Download Builder. Choose 1.13.0-pre, pick a theme you want to check & click download. The downloaded package will have an index.html file with demos of all widgets.

@fnagel
Copy link
Member

fnagel commented Sep 25, 2021

Uhhh ok, I hoped to test this before my vacation, which starts tomorrow morning, but I guess this one would take a little longer :-D

I might be able to do some testing in two weeks but no need to wait for me here. I assume I will just come to the same conclusion you did before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants