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

Make event name helper and use it on tooltip & popover to reduce dist sizes #35856

Merged
merged 3 commits into from
Feb 19, 2022

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Feb 16, 2022

No description provided.

@GeoSot GeoSot requested a review from a team as a code owner February 16, 2022 21:05
@alecpl
Copy link
Contributor

alecpl commented Feb 17, 2022

Two ideas come to mind. 1) Use eventName() in more components. 2) Get rid of EVENT_KEY.

@@ -301,7 +286,7 @@ class Tooltip extends BaseComponent {
}

this._element.removeAttribute('aria-describedby')
EventHandler.trigger(this._element, this.constructor.Event.HIDDEN)
EventHandler.trigger(this._element, this.constructor.eventName(EVENT_HIDDEN))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, but can't this be this.eventName instead of this.constructor.eventName everywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to call it statically so we can use it outside the class (at least to what I have seen till now)

@GeoSot
Copy link
Member Author

GeoSot commented Feb 17, 2022

Two ideas come to mind. 1) Use eventName() in more components. 2) Get rid of EVENT_KEY.

Totally aligned with your idea. I want to take it bit by bit to keep PR with few changes. In case it merged, it will be easier to continue change other components

@XhmikosR XhmikosR merged commit 407af8a into main Feb 19, 2022
@XhmikosR XhmikosR deleted the gs/make-event_name-helper branch February 19, 2022 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants