Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Include ThemeableMixin by default #605

Closed
wants to merge 1 commit into from

Conversation

agubler
Copy link
Member

@agubler agubler commented Jun 27, 2017

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

Proposal includes ThemeableMixin by default for WidgetBase. Renames the original WidgetBase.ts to Base.ts and adds a new WidgetBase.ts module that imports Base plus other named exports from Base.ts and ThemeableMixin and exports ThemableMixin(Base) as WidgetBase (and also re-exports all the other exports from Base.ts).

This should be backwards compatible with existing usages of WidgetBase even if they have ThemeableMixin applied.

This requires #601 to fix the failing unit test.

Resolves #581

@agubler
Copy link
Member Author

agubler commented Jun 27, 2017

Open to any better suggestions on naming :)

@kitsonk
Copy link
Member

kitsonk commented Jun 28, 2017

Not that I have been a fan of it, but it does seem to be the case here... what if we made WidgetBase and abstract class and named it Widget and make the .render() abstract. That way no one would be able to use it (in TypeScript) without extending it and implementing .render(). I think we might be scared of using Widget because people might actually use it/abuse it. Looking at a few other frameworks, they do use things like Element or Component for that kernel.

@kitsonk
Copy link
Member

kitsonk commented Jun 28, 2017

Ignore me... I didn't notice the full context that WidgetBase is still the main thing that end user will utilise.

@agubler agubler changed the title POC: Include ThemeableMixin by default Include ThemeableMixin by default Jun 28, 2017
@dylans dylans added this to the 2017.07 milestone Jul 4, 2017
@agubler
Copy link
Member Author

agubler commented Jul 27, 2017

The merges for this PR would be "tricky" so closing and will re-raise the change if there is consensus to do so.

@agubler agubler closed this Jul 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider applying core mixins to WidgetBase by default
3 participants