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

Consider applying core mixins to WidgetBase by default #581

Closed
agubler opened this issue Jun 19, 2017 · 5 comments
Closed

Consider applying core mixins to WidgetBase by default #581

agubler opened this issue Jun 19, 2017 · 5 comments

Comments

@agubler
Copy link
Member

agubler commented Jun 19, 2017

Enhancement

The main core mixins, Themeable, I18n and Registry provide functionality that could be automatically mixed in to WidgetBase by default. This would remove possible friction for consumer to need to deal with the mixin concept early on in Dojo 2 adoption.

The mixins are all additive functionality and shouldn't impact consumers who only need core WidgetBase functionality.

This would leave:

  1. Stateful, which I think we should consider removing in favour of promoting use of typed class properties for any required internal state.
  2. Projector, which would remain as an optional mixin to decorate a widget with functionality required to attach to the DOM.
@kitsonk
Copy link
Member

kitsonk commented Jun 19, 2017

I would be 👍 for Themeable and Registry. I guess how heavy is I18n overall? It feels sort of 50/50 it being fundamental. Are all @dojo/widgets using it?

I think with Stateful we don't have a compelling use case at the moment, between meta and potentially injectors with whatever "app" becomes. Agree with Projector comments.

@dylans dylans added this to the 2017.07 milestone Jul 4, 2017
@dylans dylans modified the milestones: 2017.07, 2017.08 Jul 29, 2017
@kitsonk kitsonk modified the milestones: 2017.08, 2017.09 Sep 4, 2017
@dylans dylans modified the milestones: 2017.09, 2017.10 Oct 10, 2017
@kitsonk kitsonk added beta4 and removed beta3 labels Oct 20, 2017
@kitsonk kitsonk modified the milestones: 2017.10, beta.4 Oct 30, 2017
@kitsonk
Copy link
Member

kitsonk commented Oct 30, 2017

@agubler is this still valid (undone)? I don't think it is a discussion anymore because we all think it is a 👍. If still to do, will it make beta4, or should we move it to beta5?

@agubler
Copy link
Member Author

agubler commented Oct 30, 2017

@kitsonk perhaps with the changes coming to Themeable meaning it's only needed when theming is required, not to just use classes full stop, we could possibly leave it separate.

@kitsonk
Copy link
Member

kitsonk commented Dec 1, 2017

@agubler any further action on this? It feels that with other changes we have largely mitigated this issue?

@agubler
Copy link
Member Author

agubler commented Dec 1, 2017

Completely mitigated in my opinion, closing.

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

Successfully merging a pull request may close this issue.

3 participants