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

Fixing _WidgetBase #115

Closed
wants to merge 2 commits into from
Closed

Fixing _WidgetBase #115

wants to merge 2 commits into from

Conversation

devpaul
Copy link
Member

@devpaul devpaul commented Feb 14, 2017

After noticing there were 4 _WidgetBase interfaces I did some digging and found two of them were actually misnamed definitions for _FocusMixin and _BidiMixin. I merged the other two into a single _WidgetBase interface.

Paul Shannon added 2 commits February 14, 2017 11:14
@jsf-clabot
Copy link

jsf-clabot commented Feb 14, 2017

CLA assistant check
All committers have signed the CLA.

interface _FocusMixin { }

interface _WidgetBase extends dojo.Stateful, Destroyable {
interface _FocusMixin extends _WidgetBase {
Copy link
Contributor

@mmckenziedev mmckenziedev Mar 31, 2017

Choose a reason for hiding this comment

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

According to https://github.com/dojo/dijit/blob/master/_FocusMixin.js#L57, _FocusMixin does not inherit from _WidgetBase. When the file is required, it augments _WidgetBase with a bunch of other properties. If I tried to extend from_FocusMixin and expected all the properties from _WidgetBase to be there it would not work as I expect it to.

I'm not really sure how to express this stuff properly from the definition file.

interface _FocusMixin { }

interface _WidgetBase extends dojo.Stateful, Destroyable {
interface _FocusMixin extends _WidgetBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add the focused property to the definition https://github.com/dojo/dijit/blob/master/_FocusMixin.js#L17

@devpaul devpaul closed this Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants