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

fix: fix react event name mappings BREAKING CHANGE #449

Merged
merged 4 commits into from
Mar 1, 2023

Conversation

muratcorlu
Copy link
Contributor

@muratcorlu muratcorlu commented Feb 27, 2023

While debugging #397 with @Enes5519 we realized that some of our event names in react wrappers uses reserved event names from DOM events (like, onClick, onInput, onChange). And apparently that causes to have some incompatibility issues in some cases. This PR proposes to use regular event names those defined in our web components by just converting them to PascalCase and adding on as prefix.

Also, we were dropping some parts of the event names in react event mapping. Like bl-dropdown-item-click was onClick in current implementation. That was also potentially causing confusion.

This will need to update some projects those already uses Baklava in React. But since we are just about releasing our first stable release, I think it's still a good time to do so. Please give your comments if you use Baklava in React and if this new naming convention is future proof enough in your opinion.

This PR also fixes #296 by adding type definitions for events.

PS: This doesn’t break anything for web component implementations or using it in Vue. Only effects people who use Baklava in React.

Here is a chart to compare old and new names:

Component Native Event Name Current React Event Proposed React Event
BlAlert bl-close onClose onBlClose
BlButton bl-click onClick onBlClick
BlCheckboxGroup bl-checkbox-group-change onChange onBlCheckboxGroupChange
BlDialog bl-dialog-open onOpen onBlDialogOpen
BlDialog bl-dialog-close onClose onBlDialogClose
BlDrawer bl-drawer-open onOpen onBlDrawerOpen
BlDrawer bl-drawer-close onClose onBlDrawerClose
BlDropdown bl-dropdown-open onOpen onBlDropdownOpen
BlDropdown bl-dropdown-close onClose onBlDropdownClose
BlIcon bl-load onLoad onBlLoad
BlIcon bl-error onError onBlError
BlInput bl-change onChange onBlChange
BlInput bl-input onInput onBlInput
BlInput bl-invalid onInvalid onBlInvalid
BlPagination bl-change onChange onBlChange
BlRadioGroup bl-radio-change onChange onBlRadioChange
BlSelect bl-select onSelect onBlSelect
BlSwitch bl-switch-toggle onToggle onBlSwitchToggle
BlTextarea bl-input onInput onBlInput
BlTextarea bl-change onChange onBlChange
BlTextarea bl-invalid onInvalid onBlInvalid
BlTooltip bl-tooltip-show onShow onBlTooltipShow
BlTooltip bl-tooltip-hide onHide onBlTooltipHide
BlCheckbox bl-checkbox-change onChange onBlCheckboxChange
BlCheckbox bl-focus onFous onBlFocus
BlCheckbox bl-blur onBlur onBlBlur
BlDropdownItem bl-dropdown-item-click onClick onBlDropdownItemClick
BlRadio bl-checked onChecked onBlChecked
BlRadio bl-focus onFocus onBlFocus
BlRadio bl-blur onBlur onBlBlur
BlSelectOption bl-select-option onOption onBlSelectOption
BlTab bl-tab-selected onSelected onBlTabSelected

Fixes #397
Fixes #307

BREAKING CHANGE: this commit adds prefixes to the event names in React integration
also pascal case dependency changed with a more popular library
@ozkersemih
Copy link
Contributor

I think pr seems ok. Good job :)

@AykutSarac
Copy link
Member

PS: This doesn’t break anything for web component implementations or using it in Vue. Only effects people who use Baklava in React.

I'd recommend allowing previous events with the @deprecated flag, otherwise this change will likely to be adopted very late by the shell app has microfrontends and this will result in Baklava getting less feedbacks from community.

@muratcorlu
Copy link
Contributor Author

I'd recommend allowing previous events with the @deprecated flag, otherwise this change will likely to be adopted very late by the shell app has microfrontends and this will result in Baklava getting less feedbacks from community.

Because of we fetch our web components from CDN and React wrappers from NPM, increasing the CDN version in app shell, will not effect React events automatically. Every React app will need to update their NPM dependency separate. So I think the only risk is the pople that uses latest beta version in their package.json (which we suggest to avoid).

@Enes5519
Copy link
Contributor

It's still bad in type, but it solves our problem.

@muratcorlu muratcorlu merged commit 0254540 into next Mar 1, 2023
@muratcorlu muratcorlu deleted the react-event-mapping-fixes branch March 1, 2023 08:38
@github-actions
Copy link

github-actions bot commented Mar 1, 2023

🎉 This PR is included in version 2.0.0-beta.77 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Apr 6, 2023

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants