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

All: Classes Option #1369

Closed
wants to merge 47 commits into from
Closed

All: Classes Option #1369

wants to merge 47 commits into from

Conversation

arschmitz
Copy link
Member

This replaces PR #790 It updates the widget factory to handle the classes option via setOption, and updates all the widgets to use the _elementsFromClassKey method to set what elements belong to what classkeys for programmatic updating. It also updates all classes options to use "" instead of null to make it easier to concant new classes.

options: {
classes: {
"test": "class1 class2",
"test2": "class3"
Copy link
Member

Choose a reason for hiding this comment

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

This should include one property with the empty string as the value.

Choose a reason for hiding this comment

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

#1372 adds that test.

@jzaefferer
Copy link
Member

I reviewed parts of this. Not quite done, but since I found a few things that can be addressed, I'll continue after the next update, or whenever I can get back to this.

@jzaefferer
Copy link
Member

tests/visual/tooltip/tooltip.html and demos/autocomplete/combobox.html still use tooltipClass.

@jzaefferer
Copy link
Member

Some small issues here and there, but overall looks good. Would be nice to simplify _elementsFromClassKey further, since that adds the most code, but I don't know how. @scottgonzalez @tjvantoll @petersendidit @gabrielschulhof any ideas?

@arschmitz
Copy link
Member Author

@jzaefferer This is fully updated and rebased with master only thing I didn't address is the test you questioned that @petersendidit answered about if that should stay or not.

@jzaefferer
Copy link
Member

Thanks, I'll review this again asap. One thing I wanted to mention though:

We should have at least one demo highlighting this new feature, preferably based on some of the original usecases. The only I can think of is "customising where rounded corners are used". This could also demo some other options for "custom theming", like some simple overrides to change colors. Just to give an idea that it's possible.

@arschmitz
Copy link
Member Author

@jzaefferer yeah demos would be good corners is the very basic simple use case we could also do something more complex like using it to apply another theme completely like foundation or bootstrap?

@tjvantoll
Copy link
Member

I think Scott's example of using Bootstrap's CSS with the UI dialog would make for a good demo.

@arschmitz
Copy link
Member Author

Added the basic corner class demos working on demos with a different theme

@jzaefferer
Copy link
Member

We discussed at the meeting last week that the demo commits should be reverted. With a rebase they can be dropped, no need to add to the pile.

@tjvantoll wants to write a learn article about using classes instead, that should be more useful.

@jzaefferer
Copy link
Member

Demo commits are gone. Learn article can we written later. I think this is good to land.

@scottgonzalez
Copy link
Member

I know it's late in the process, but I just had an idea that I discussed with @arschmitz in IRC:

If we have this._addClass( elems, key, extraClasses and this._removeClass( elems, key, extraClasses ), then the base widget can do all the tracking and the need for individual widgets to implement methods like _elementsFromClassKey() would go away. This should simplify the implementation of individual widgets, which will reduce the number of potential bugs and also save on file size.

@arschmitz said he'll work on implementing that to see how it goes.

@arschmitz arschmitz closed this Dec 11, 2014
@jzaefferer
Copy link
Member

Replaced by #1392

@arschmitz arschmitz deleted the classes branch January 8, 2015 03:56
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.

6 participants